code review 4005045: Make Walk webbrowser example work.

Make Walk webbrowser example work.

[hector, 2011-01-22 21:36:01]

Hello rsc, brainman (cc: golang-dev@googlegroups.com, lxn),

I'd like you to review this change.

[lxn, 2011-01-22 23:51:13]

Hi Hector, 

thanks for looking into this!

With your changes the WebView stuff without events works (like it did with a
patch from Mateusz Czapliński). After reactivating the WebView event code, it
crashes again. It happens at the same location as was the case with the patch
from Mateusz.

Not sure if the problem is in Walk or the runtime. I never did any real COM work
besides playing with DirectX many years ago, so I probably got something wrong.

Cheers
Alex

[brainman, 2011-01-23 01:04:14]

LGTM

http://codereview.appspot.com/4005045/diff/6001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):

http://codereview.appspot.com/4005045/diff/6001/src/pkg/runtime/386/asm.s#new...
src/pkg/runtime/386/asm.s:50: LEAL	(-64*1024+104)(SP), AX	// TODO: 104?
I assume, you had to increase this because you have multiple callbacks one
inside another. How did you decide that 64k is enough? Maybe we should just make
it really big. I'm not sure what is "really big". What happens if we have (I
can't see how) very long chain of callbacks. Perhaps the way we call callbacks
should be changed?

http://codereview.appspot.com/4005045/diff/6001/src/pkg/runtime/windows/386/s...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4005045/diff/6001/src/pkg/runtime/windows/386/s...
src/pkg/runtime/windows/386/sys.s:96: // EBX, EBP, ESI, EDI registers and DF
flag are preserved
Do you have any references that describe this rules?

[hector, 2011-01-23 13:08:02]

Hi Alex N,

We need to change syscall.NewCallback to take argsize in terms of number of
words.  If you will observe:

syscall.NewCallback(webView_DWebBrowserEvents2_Invoke, 4+4+4+4+2+4+4+4+4)

The argsize here adds up to 34.  The stack pointer will be adjusted by this
number, causing it to lose word alignment.  This causes all manner of mischief.

What needs to be done is to change all the argsizes into number of words.  For
instance in the above code the argsize would be 9.  Rest assured, if an argument
is defined as a WORD that it will still use up a DWORD space on the stack.

After you have done that it will still not work, because the order of the
functions in the IConnectionPointVtbl is incorrect.  The following is the
correct definition:

type IConnectionPointVtbl struct {
	QueryInterface              uintptr
	AddRef                      uintptr
	Release                     uintptr
	GetConnectionInterface      uintptr
	GetConnectionPointContainer uintptr
	Advise                      uintptr
	Unadvise                    uintptr
	EnumConnections             uintptr
}

Alex B, I simply increased the stack size until it no longer crashed.  With
regards to preserving EBP, I couldn't find a reference on the Web, but it has to
be done because the Windows function calling us uses it.

[hector, 2011-01-23 13:21:41]

Sorry, I forgot, PTAL.

[lxn, 2011-01-23 17:03:08]

Hector,

Thanks for analyzing this.

I just pushed an update to Walk that should fix the things you have pointed out.
But with your latest changes on top of a fresh clone of Go tip, filebrowser and
webbrowser examples just exit without any message, while the others work. The
filebrowser example now suddenly failing seems weird.

Alex

[hector, 2011-01-23 18:07:58]

You forgot to change these two:

diff --git a/gui/lineedit.go b/gui/lineedit.go
index 518580a..f27ac58 100644
--- a/gui/lineedit.go
+++ b/gui/lineedit.go
@@ -43,7 +43,7 @@ func NewLineEdit(parent IContainer) (*LineEdit, os.Error) {
 	}
 
 	if lineEditSubclassWndProcCallback == nil {
-		lineEditSubclassWndProcCallback =
syscall.NewCallback(lineEditSubclassWndProc, 4+4+4+4)
+		lineEditSubclassWndProcCallback =
syscall.NewCallback(lineEditSubclassWndProc, 4)
 		lineEditSubclassWndProcPtr =
uintptr(lineEditSubclassWndProcCallback.ExtFnEntry())
 	}
 
diff --git a/gui/listview.go b/gui/listview.go
index 47537cc..1ff0abc 100644
--- a/gui/listview.go
+++ b/gui/listview.go
@@ -52,7 +52,7 @@ func NewListView(parent IContainer) (*ListView, os.Error) {
 	}
 
 	if listViewSubclassWndProcCallback == nil {
-		listViewSubclassWndProcCallback =
syscall.NewCallback(listViewSubclassWndProc, 4+4+4+4)
+		listViewSubclassWndProcCallback =
syscall.NewCallback(listViewSubclassWndProc, 4)
 		listViewSubclassWndProcPtr =
uintptr(listViewSubclassWndProcCallback.ExtFnEntry())
 	}

[lxn, 2011-01-23 18:26:22]

On 2011/01/23 18:07:58, hector wrote:
> You forgot to change these two:
> 
> diff --git a/gui/lineedit.go b/gui/lineedit.go
> index 518580a..f27ac58 100644
> --- a/gui/lineedit.go
> +++ b/gui/lineedit.go
> @@ -43,7 +43,7 @@ func NewLineEdit(parent IContainer) (*LineEdit, os.Error) {
>  	}
>  
>  	if lineEditSubclassWndProcCallback == nil {
> -		lineEditSubclassWndProcCallback =
> syscall.NewCallback(lineEditSubclassWndProc, 4+4+4+4)
> +		lineEditSubclassWndProcCallback =
> syscall.NewCallback(lineEditSubclassWndProc, 4)
>  		lineEditSubclassWndProcPtr =
> uintptr(lineEditSubclassWndProcCallback.ExtFnEntry())
>  	}
>  
> diff --git a/gui/listview.go b/gui/listview.go
> index 47537cc..1ff0abc 100644
> --- a/gui/listview.go
> +++ b/gui/listview.go
> @@ -52,7 +52,7 @@ func NewListView(parent IContainer) (*ListView, os.Error) {
>  	}
>  
>  	if listViewSubclassWndProcCallback == nil {
> -		listViewSubclassWndProcCallback =
> syscall.NewCallback(listViewSubclassWndProc, 4+4+4+4)
> +		listViewSubclassWndProcCallback =
> syscall.NewCallback(listViewSubclassWndProc, 4)
>  		listViewSubclassWndProcPtr =
> uintptr(listViewSubclassWndProcCallback.ExtFnEntry())
>  	}

D'oh!

Thanks again, it works great now :)

[brainman, 2011-01-23 23:01:20]

On 2011/01/23 13:08:02, hector wrote:
> ...  Rest assured, if an argument
> is defined as a WORD that it will still use up a DWORD space on the stack.
> ...

Very good idea. I was wrongly assuming all arguments will have 4 byte sizes.

Alex

[rsc, 2011-01-24 19:18:10]

LGTM

[brainman, 2011-01-25 06:56:43]

*** Submitted as http://code.google.com/p/go/source/detail?r=a41f79bb01ea ***

runtime: make Walk webbrowser example work

R=rsc, brainman, lxn
CC=golang-dev
http://codereview.appspot.com/4005045

Committer: Alex Brainman <alex.brainman@gmail.com>