503877 - Remove nsSwitchToUIThread

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Rob Arnold [:robarnold]]

Attachment: win32 patch

It appears to be unused and unnecessary for OS/2 and Windows.

Comment 1

[Rob Arnold [:robarnold]]

Attachment: os/2 patch

Also want to do this to os/2 (which is similar to the windows widget backend) so that bug 503879 can remove more code.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

How sure are you that this can go away?  Would we ever receive a destroy message on the wrong thread?

(Have you looked at where this code came from?)

Comment 3

[Rob Arnold [:robarnold]]

This code has been there since day 1. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=1.1&root=/cvsroot

I don't think we ever get calls to widget on other threads, given how thread adverse xpcom (nsIToolkit/nsIWidget) and the rest of the codebase are. Also, this interface doesn't exist on the newer cocoa and gtk2 backends so I suspect it has not been used or needed for quite some time.

Comment 4

[Peter Weilbacher]

Comment on attachment 388231 [details] [diff] [review]
os/2 patch

> MRESULT EXPENTRY nsToolkitWindowProc(HWND hWnd, ULONG msg, MPARAM mp1,
>                                      MPARAM mp2)
> {
>-    switch (msg) {
>-        case WM_CALLMETHOD:
>-        {
>-            MethodInfo *info = (MethodInfo *)mp2;
>-            return (MRESULT)info->Invoke();
>-        }
>-    }
>+    // XXX: is this needed anymore?
>     return ::WinDefWindowProc(hWnd, msg, mp1, mp2);

I don't think it is still needed. I completely removed this function and used NULL instead in the class registration call. That didn't seem to change anything.

>--- a/widget/src/os2/nsWindow.h
>+++ b/widget/src/os2/nsWindow.h
[...]
> class nsWindow : public nsBaseWidget,
>-                 public nsSwitchToUIThread

Please remove the comma at the end of the line.

Comment 5

[Peter Weilbacher]

Rich, do you have a deeper understanding of the nsToolkit class on OS/2 to comment on the necessity of the nsToolkitWindowProc() function?

I have only run an OS/2 build with this patch for 20 minutes or so, and I'm a bit surprised that it didn't seem to have any effect. But if it works, this is great cleanup! :-)

Comment 6

[Rich Walsh]

(In reply to comment #5)
> Rich, do you have a deeper understanding of the nsToolkit class on
> OS/2 to comment on the necessity of the nsToolkitWindowProc() function?

Not really.  I did some basic testing (viewed various pages & dialogs, dragged tabs off to open a new window, watched Flash & Ogg videos, etc) and never once saw the toolkit window proc invoked.  I also confirmed that Firefox never creates more than one message queue (on thread 1, as expected).

Some comments on the patch:
- the OS/2 nsToolkitWindowProc() is no longer used & should be eliminated
- it might not be a bad idea to include an assertion or whatever in Create() and Destroy() to ensure that they're being invoked on thread 1 (I assume that TID 1 will remain the one & only GUI thread).
- to forestall future problems, it should be clearly documented (especially in the Win32 version) that the GUI functions formerly handled by toolkit must be invoked from thread 1 only

Comment 7

[Rob Arnold [:robarnold]]

(In reply to comment #6)
> Some comments on the patch:
> - the OS/2 nsToolkitWindowProc() is no longer used & should be eliminated

Will do, gladly.

> - it might not be a bad idea to include an assertion or whatever in Create()
> and Destroy() to ensure that they're being invoked on thread 1 (I assume that
> TID 1 will remain the one & only GUI thread).

I'm not sure if this is necessary. All of widget/ (well ok, maybe not BeOS) is written to assume a single thread and it would probably not be beneficial to write this assert.

> - to forestall future problems, it should be clearly documented (especially in
> the Win32 version) that the GUI functions formerly handled by toolkit must be
> invoked from thread 1 only

I think it's just understood by convention/a lack of locks (this is true for most of the mozilla codebase). All our widget backends work this way. This code is buggy anyways - there's a race condition in the initialization of the TLS index for nsToolkit.

Comment 8

[Rob Arnold [:robarnold]]

Windows patch pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/da889626b008

OS/2 patch and bug expanded to bug 508397