163918 - crash opening unrequested popups [@ nsWatcherWindowEnumerator::GetNext][@ GlobalWindowImpl::QueryInterface] [@ 0x00000000]

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Dan M]

1) Set prefs to disable opening unrequested windows
2) Open and close at least one window (opening the prefs window to set the pref
will suffice)
3a) Start browsing sites that have unrequested popups in named windows -or-
3b) Do that by loading the attached test case. Repeat until crash

Comment 1

[Dan M]

Attachment: test case: onload opens a named popup window

Comment 2

[Dan M]

see also bug 163914, which may be the same

Comment 3

[Dan M]

*** Bug 163672 has been marked as a duplicate of this bug. ***

Comment 4

[greer]

Moving over the info from bug 163672.

Comment 5

[Robert Ginda]

*** Bug 163775 has been marked as a duplicate of this bug. ***

Comment 6

[Sobrinho]

*** Bug 163936 has been marked as a duplicate of this bug. ***

Comment 7

[Adam Lock]

Here is a stack trace which looks different from bug 163914. 

 	embedcomponents.dll!CallQueryInterface(nsIDOMWindow * aSource=0x0392856c,
nsISupports * * aDestination=0x0012d308)  Line 271	C++
>
embedcomponents.dll!nsWatcherWindowEnumerator::GetNext(nsISupports * *
retval=0x0012d308)  Line 241 + 0x12	C++
 	embedcomponents.dll!nsWindowWatcher::FindItemWithName(const unsigned short *
aName=0x0012d474, nsIDocShellTreeItem * * aFoundItem=0x0012d3f8)  Line 1383	C++
 	embedcomponents.dll!nsWindowWatcher::GetWindowByName(const unsigned short *
aTargetName=0x0012d474, nsIDOMWindow * aCurrentWindow=0x04cf20f4, nsIDOMWindow *
* aResult=0x0012d460)  Line 1034 + 0x24	C++
 	jsdom.dll!GlobalWindowImpl::Open(nsIDOMWindow * * _retval=0x0012d7a8)  Line 2945 +
0x77
C++
 	xpcom.dll!XPTC_InvokeByIndex(nsISupports * that=0x04cf20f8, unsigned int
methodIndex=15, unsigned int paramCount=1, nsXPTCVariant * params=0x0012d7a8)  Line
106
C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 1994 + 0x2a	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x04cf1260, JSObject *
obj=0x04cc4c20, unsigned int argc=2, long * argv=0x058a007c, long *
vp=0x0012da84)  Line 1266 + 0xe	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x04cf1260, unsigned int argc=2, unsigned
int flags=0)  Line 838 + 0x17	C
 	js3250.dll!js_Interpret(JSContext * cx=0x04cf1260, long * result=0x0012e8c4) 
Line 2801 + 0xf	C
 	js3250.dll!js_Invoke(JSContext * cx=0x04cf1260, unsigned int argc=1, unsigned
int flags=2)  Line 855 + 0xd	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x04cf1260, JSObject *
obj=0x04cc4c20, long fval=80495416, unsigned int flags=0, unsigned int argc=1,
long * argv=0x0012eb1c, long * rval=0x0012e9ec)  Line 930 + 0x14	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x04cf1260, JSObject *
obj=0x04cc4c20, long fval=80495416, unsigned int argc=1, long * argv=0x0012eb1c,
long * rval=0x0012e9ec)  Line 3431 + 0x1f	C
 	jsdom.dll!nsJSContext::CallEventHandler(void * aTarget=0x04cc4c20, void *
aHandler=0x04cc4338, unsigned int argc=1, void * argv=0x0012eb1c, int *
aBoolResult=0x0012eb20, int aReverseReturnResult=0)  Line 1041 + 0x21	C++
 	jsdom.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x05537018)  Line
182 + 0x4d	C++
 	gkcontent.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct *
aListenerStruct=0x055efa58, nsIDOMEvent * aDOMEvent=0x05537018,
nsIDOMEventTarget * aCurrentTarget=0x04cf2100, unsigned int aSubType=1, unsigned
int aPhaseFlags=7)  Line 1182 + 0x14	C++
 	gkcontent.dll!nsEventListenerManager::HandleEvent(nsIPresContext *
aPresContext=0x057ccf18, nsEvent * aEvent=0x0012f24c, nsIDOMEvent * *
aDOMEvent=0x0012f208, nsIDOMEventTarget * aCurrentTarget=0x04cf2100, unsigned
int aFlags=7, nsEventStatus * aEventStatus=0x0012f274)  Line 1851 + 0x24	C++
 	jsdom.dll!GlobalWindowImpl::HandleDOMEvent(nsIPresContext *
aPresContext=0x057ccf18, nsEvent * aEvent=0x0012f24c, nsIDOMEvent * *
aDOMEvent=0x0012f208, unsigned int aFlags=1, nsEventStatus *
aEventStatus=0x0012f274)  Line 764	C++
 	gkcontent.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line 928 +
0x2f
C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x04c1794c,
nsIChannel * aChannel=0x05538ad0, unsigned int aStatus=0)  Line 4165	C++
 	docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x04c1794c,
nsIChannel * channel=0x05538ad0, unsigned int aStatus=0)  Line 814	C++
 	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x04c1794c,
nsIRequest * aRequest=0x05538ad0, unsigned int aStateFlags=131088, unsigned int
aStatus=0)  Line 4079	C++
 	urildr.dll!nsDocLoaderImpl::FireOnStateChange(nsIWebProgress *
aProgress=0x04c1794c, nsIRequest * aRequest=0x05538ad0, int aStateFlags=131088,
unsigned int aStatus=0)  Line 1235	C++
 	urildr.dll!nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * request=0x05538ad0,
unsigned int aStatus=0)  Line 870	C++
 	urildr.dll!nsDocLoaderImpl::DocLoaderIsEmpty()  Line 768	C++
 	urildr.dll!nsDocLoaderImpl::OnStopRequest(nsIRequest * aRequest=0x0562f398,
nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 699	C++
 	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x0562f398,
nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 694 + 0x23	C++
 	gklayout.dll!PresShell::RemoveDummyLayoutRequest()  Line 6630 + 0x2a	C++
 	gklayout.dll!PresShell::DoneRemovingReflowCommands()  Line 6587	C++
 	gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=1)  Line 6446	C++
 	gklayout.dll!ReflowEvent::HandleEvent()  Line 6234	C++
 	gklayout.dll!HandlePLEvent(ReflowEvent * aEvent=0x057f11b0)  Line 6248	C++
 	xpcom.dll!PL_HandleEvent(PLEvent * self=0x057f11b0)  Line 596 + 0xa	C
 	xpcom.dll!PL_ProcessPendingEvents(PLEventQueue * self=0x010fb460)  Line 526 + 0x9	C
 	xpcom.dll!_md_EventReceiverProc(HWND__ * hwnd=0x00050234, unsigned int
uMsg=49470, unsigned int wParam=0, long lParam=17806432)  Line 1077 + 0x9	C
 	USER32.DLL!77d43a5f() 	
 	USER32.DLL!77d43b2e() 	
 	USER32.DLL!77d43d6a() 	
 	USER32.DLL!77d441fd() 	
 	appshell.dll!nsAppShellService::Run()  Line 452	C++
 	mozilla.exe!main1(int argc=1, char * * argv=0x002a6e40, nsISupports *
nativeApp=0x00000000)  Line 1507 + 0x20	C++
 	mozilla.exe!main(int argc=1, char * * argv=0x002a6e40)  Line 1871 + 0x25	C++
 	mozilla.exe!mainCRTStartup()  Line 338 + 0x11	C
 	KERNEL32.DLL!77e7eb69() 	

Comment 8

[Andrew Schultz]

regression between 2002081905 and 2002082008

Comment 9

[Andrew Schultz]

this is a regression from bug 130719

Comment 10

[Andrew Schultz]

*** Bug 163870 has been marked as a duplicate of this bug. ***

Comment 11

[Jerry Baker]

*** Bug 163969 has been marked as a duplicate of this bug. ***

Comment 12

[Jerry Baker]

Talkback reports if wanted:

TB9672330Z
TB9666583G
TB9666427G
TB9661369E

Comment 13

[Andrew Schultz]

*** Bug 163871 has been marked as a duplicate of this bug. ***

Comment 14

[Frankie]

*** Bug 164039 has been marked as a duplicate of this bug. ***

Comment 15

[Frankie]

Attachment: crash log Aug 22

Submitting crash log since OS X doesn't have Talkback.

Comment 16

[greer]

*** Bug 164029 has been marked as a duplicate of this bug. ***

Comment 17

[greer]

Some stacks make it one frame farther before crashing at the
GlobalWindowImpl::QueryInterface signature. Adding to the summary for Talkback.

Comment 18

[Henrik Gemal]

*** Bug 163986 has been marked as a duplicate of this bug. ***

Comment 19

[Alec Flett]

this has quickly risen through the topcrash rank to become the number 1 topcrash
in the last 2 days. 290 crashes in 2 days, vs 83 crashes for the #2, and 46
crashes for the #3

Comment 20

[serge (gone)]

*** Bug 164079 has been marked as a duplicate of this bug. ***

Comment 21

[Dan M]

Attachment: fix dangling references to closed windows in WindowWatcher

Basically all this patch does is breaks the nsAppShellService method which
once unregistered windows and also then tried to quit the app into two separate
methods, so they can be called in two separate places. Then the former can be
called early enough during window destruction that WindowWatcher doesn't become
confused.
  The most logical place to put the "attempt to quit" code is in the Quit
method, so that now takes a parameter. (This is a change to an (unfrozen)
interface, and it also necessitated changing every caller.) While I'm in there
I've re-enabled the ability to force quit, which was recently lost (see bug
130719). People seemed to miss it, though I'm not sure when it should be used.
But at least it's in the interface. I also had to remove an (unused) parameter
from nsNativeAppSupport::OnLastWindowClosing because it's now being used in a
place where that information is no longer available.
  And while I was in nsNativeAppSupportOS2.cpp I brought it up to speed like
the Windows version (see bug 163718).

Comment 22

[Andrew Schultz]

*** Bug 164104 has been marked as a duplicate of this bug. ***

Comment 23

[Bill Law]

Comment on attachment 96359 [details] [diff] [review]
fix dangling references to closed windows in WindowWatcher

r=law, but only for the nsNativeAppSupportWin and nsKillAll changes.  I'm not
checking the "has-review" box because those changes are a relatively small part
of this patch.

Comment 24

[timeless]

offhand i'd bet this should be eConsiderQuit
+  if (aFerocity == eForceQuit)
+    return NS_ERROR_FAILURE;
... you can't reach this second if w/ aFerocity == eForceQuit
something looks messed up.
+  if (aFerocity == eAttemptQuit || aFerocity == eForceQuit) {
...
... you still can't reach an if w/ like aFerocity == eForceQuit
something is messed up.
+  if (aFerocity == eForceQuit) {
...
+          PL_InitEvent(NS_REINTERPRET_CAST(PLEvent*, event),
+                        nsnull,
+                        HandleExitEvent,
+                        DestroyExitEvent);
please make this lineup (it's not your fault, but you're moving it around)

i'd argue needs work :-)

Comment 25

[Mats Palmgren (inactive)]

*** Bug 164107 has been marked as a duplicate of this bug. ***

Comment 26

[timeless]

fwiw i've sent some of these crashes, and i didn't have the unrequested popups
pref set, i was using a js script in jsconsole which opened lots of open dialogs
and then i would try to run venkman.

Comment 27

[Dan M]

  timeless:
  (comment 24)
  aFerocity works its way up to eForceQuit from less ferocious values as it
works its way through the Quit method, succeeding at each step. See lines 142
and 214 of the patch. The method refuses to take an initial value of eForceQuit
because *forcing* a quit can still cause the bug 115969 crash. It could detect
the problematic situation later on and bail further in, but I wanted to make it
explicit that that's not really supported yet. No one is trying to use the
option yet, of course.
  That initial check shouldn't refuse eConsiderQuit. That's a valid initial
value and is being used a lot in the patch.
  I can make the PL_InitEvent stuff line up, of course. However, I think the
logic flaws you've detected are not actually a problem.
  (comment 26) This bug has deep roots. When I first heard about it I knew what
it was and tried to figure out how it could possibly happen and found after much
searching only the one path that I reported in the initial comment of this bug.
As far as I know that's the only way. But I'm probably wrong. At least it led to
a verifiable test case.

Comment 28

[Dan M]

  Oh. I misunderstood comment 24. Yes, the second aFerocity == eForceQuit check
can't be reached as the code stands because of the first check. However, that
first check is temporary; an explicit indication that that option isn't
supported. Some day if it is, all that needs be done is remove that first check.
Logically, the code in the second big if clause (if aFerocity == eAttemptQuit ||
aFerocity == eForceQuit) wants to be executed in the eForceQuit case. As you
correctly pointed out, it can't be reached yet. But it will some day and I'd
hate to forget to add that check when I took out the first one.
  I'll add a comment to the first check to make this more clear.

Comment 29

[Damir Perisa]

what about disabling the ability to accept unrequested popups?

the only use of UNREQUESTED popups is for advertisments, isn't it?

Comment 30

[Ben Guthrie]

are you going to back out whatever change created this bug until a solution is
agreed upon or should we users fall back to browsers from earlier in the week.

It's imposible to use the current Mozilla with the constant crashes.  Especially
since I've been pop-up free for so long, I have no idea which sites to avoid.

Comment 31

[WD]

*** Bug 164251 has been marked as a duplicate of this bug. ***

Comment 32

[Dan M]

  Ben: I understand your frustration. We do have a fix ready to go in. It's not
a contentious fix; no genuine flaws have yet been pointed out. Hope to get it in
today.
  Damir: depends on what you mean by "unrequested." Mozilla can't tell the
difference between spam and legitimate popups like "do you want to save your
work?" Both are unrequested. And both are controlled by the Preferences->
Advanced-> Scripts & Plugins-> (Allow webpages to) Open Unrequested Windows pref.

Comment 33

[Steffen Motzer]

*** Bug 164269 has been marked as a duplicate of this bug. ***

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 96359 [details] [diff] [review]
fix dangling references to closed windows in WindowWatcher

>+  const unsigned short eConsiderQuit = 1; // attempt to quit if all windows are closed
>+  const unsigned short eAttemptQuit = 2;  // try to close all windows, then quit if successful
>+  const unsigned short eForceQuit = 3;    // quit, damnit
>+
>   /**
>    * Required initialization routine.
>    * @param aCmdLineService is stored and passed to appshell components as
>@@ -84,9 +88,9 @@
>   void run();
> 
>   /**
>-   * Exit the event loop
>+   * Exit the event loop, shut down the app
>    */
>-  void quit();
>+  void quit(in unsigned short aFerocity);

Why use unsigned short instead of unsigned long?  Better yet, ensure that
nsrootidl.idl is included and use PRInt32 or PRUint32.

>+  if (aFerocity == eForceQuit)
>+    return NS_ERROR_FAILURE;
>+
>+      // Check to see if we should quit in this case.
>+      if (mNativeAppSupport) {
>+        PRBool serverMode = PR_FALSE;
>+        mNativeAppSupport->GetIsServerMode(&serverMode);
>+        if (serverMode) {
>+          // stop! give control to server mode
>+          mShuttingDown = PR_FALSE;
>+          mNativeAppSupport->OnLastWindowClosing();
>+          return NS_OK;
>+        }
>       }

How many names do we have for QuickLaunch?  Turbo, ServerMode, ...?

>+  if (aFerocity == eAttemptQuit || aFerocity == eForceQuit) {

timeless was right -- comment the apparent conundrum, and maybe even comment
out this || second operand.

>+      if (windowEnumerator) {
>+
>+        while (1) {
>+          PRBool more;
>+          rv = windowEnumerator->HasMoreElements(&more);
>+          if (NS_FAILED(rv) || !more)
>+            break;

Nit: Maybe a 'while (!NS_FAILED(windowEnumerator->HasMoreElements(&more)) ||
!more) {...}' would be better.

>+        /* did we close all windows? if not, we can't quit yet. but that's
>+          alright, we started windows shutting down. when the last one goes,
>+          we'll come back. */

Is it the case that we tried, but failed, to close a window?  Or is this only
the case where onunload calls window.open?  Comment if so.

>+    // first shutdown native app support; doing this first will prevent new
>+    // requests to open additional windows coming in.
>+    if (mNativeAppSupport) {
>+      mNativeAppSupport->Quit();
>+      mNativeAppSupport = 0;

Nit, depending on local style: nsnull rather than 0?

>+    static PRBool mInitialWindowActive;
>     static PRBool mForceProfileStartup;
>     static char mMutexName[];
>     static PRBool mUseDDE;

PRPackedBool for these three, and whatever others are adjacent in static data
space.

>-    mInitialWindow = newWindow;
>+    mInitialWindowActive = PR_TRUE;

Yay, a true boolean!

Fix nits and sr=[e-mail] (I trust you'll get a good r=).

/be

Comment 35

[Brian Ryner (not reading)]

Comment on attachment 96359 [details] [diff] [review]
fix dangling references to closed windows in WindowWatcher

I'm going to play devil's advocate here... what if a window sets a timer in the
onunload handler and the timer callback opens a window?  Will the timer be
destroyed with the window and hence never fire, or will that get us back into
this situation?

Also, uber-nit: "focussing" -> "focusing".

Comment 36

[Dan M]

Spelling pride makes me point out that "focussing" wasn't mine but I'll take
this opportunity to fix it. Opening a window from a timeout set in an unload
handler would be an issue with the original crash, bug 115969. I'm really just
trying to get this patch checked in so I can fix a different crash. But just for
fun I tried it. Set a timeout, even one with time 0, in an unload handler and
the timeout never fires. As it should. Timeouts are cleared during window
destruction.

Comment 37

[Brian Ryner (not reading)]

Comment on attachment 96359 [details] [diff] [review]
fix dangling references to closed windows in WindowWatcher

r=bryner

Comment 38

[Andrew Schultz]

*** Bug 164350 has been marked as a duplicate of this bug. ***

Comment 39

[Andrew Schultz]

*** Bug 164333 has been marked as a duplicate of this bug. ***

Comment 40

[Frankie]

I'm glad this is almost fixed. As a mere mortal end user, it's interesting to
watch the debug process. For now I'm using latest-1.1 instead of latest-trunk. I
highly recommend it to Ben, Damir, etc.

http://ftp.mozilla.org/pub/mozilla/nightly/latest-1.1/

Comment 41

[Jo Hermans]

*** Bug 164391 has been marked as a duplicate of this bug. ***

Comment 42

[Alec Flett]

damir: that is a different issue, and mozilla already has that ability..
Edit->Preferences->Advanced->Scripts & Plugins

Comment 43

[Damir Perisa]

in risking to be off topic: i know the feature mentioned in comment 42 , but "unrequested windows" can be splitt in 2 groups: -unrequested windows (normal browser windows, that open unrequested) -unrequested popups  (browser windows, that have no toolbars or statusbars,                       just the content that has to be rendered) i dont know if this difference can be made for the behaviour they are inter- preted - maybe there is no difference in the request made by the script at all - but if there is one, they must be interpreted differently --- the first case is not frequent on the internet, so maybe this bug is only existant for the second case   sorry, if i disturb the work/discussion done in this bug 

Comment 44

[Jo Hermans]

*** Bug 164520 has been marked as a duplicate of this bug. ***

Comment 45

[Dan M]

Fixed in trunk (the bug doesn't happen on the 1.0 branch). This morning's builds
will have the fix.

Just to be clear, step 3b of the reliable testcase mentioned in this bug's
initial comment is, after loading the testcase page, to hit the Reload button
over and over. This morning's build is safe; a build from late last week will
crash on something like the second or fourth reload.

Comment 46

[greer]

*** Bug 164650 has been marked as a duplicate of this bug. ***

Comment 47

[greer]

*** Bug 164699 has been marked as a duplicate of this bug. ***

Comment 48

[Andrew Schultz]

*** Bug 164691 has been marked as a duplicate of this bug. ***

Comment 49

[Ben Guthrie]

I haven't had a crash in 98 or NT since Sunday's latest.
Thank You to everyone who worked on this one.

Comment 50

[Jay Patel [:jay]]

After seeing 200-300 crashes a day from 8/20-8/24, there have been 0 crashes
with MozillaTrunk builds as of 8/25.  Marking verified based on Talkback data.

Comment 51

[Ajay Gautam]

I would like to thank the mozilla team for getting this out of the way.

It will go a long way towards my (customer) satisfaction.

Thanks :)

Ajay

Comment 52

[Boris Zbarsky [:bzbarsky]]

*** Bug 167568 has been marked as a duplicate of this bug. ***

Comment 53

[Torben]

*** Bug 163957 has been marked as a duplicate of this bug. ***