It might be a duplicate of bug 814953. It's currently #12 top browser crasher in 19.0a2 with many dupes. Signature nsWindow::DealWithPopups(HWND__*, unsigned int, unsigned int, long, long*) More Reports Search UUID 49b8c984-4cc8-4398-9809-fbf2c2121206 Date Processed 2012-12-06 16:24:48 Uptime 47 Last Crash 19.3 hours before submission Install Age 9.4 hours since version was first installed. Install Time 2012-12-06 07:00:48 Product Firefox Version 19.0a2 Build ID 20121205042014 Release Channel aurora OS Windows NT OS Version 6.1.7600 Build Architecture x86 Build Architecture Info GenuineIntel family 6 model 15 stepping 13 Crash Reason EXCEPTION_ACCESS_VIOLATION_READ Crash Address 0x0 User Comments Processor Notes WARNING: JSON file missing Add-ons EMCheckCompatibility False Total Virtual Memory 2147352576 Available Virtual Memory 2002739200 System Memory Use Percentage 31 Available Page File 3538944000 Available Physical Memory 1457876992 Frame Module Signature Source 0 xul.dll nsWindow::DealWithPopups widget/windows/nsWindow.cpp:7952 1 xul.dll nsWindow::WindowProcInternal widget/windows/nsWindow.cpp:4338 2 xul.dll CallWindowProcCrashProtected xpcom/base/nsCrashOnException.cpp:32 3 xul.dll xul.dll@0x2aac1f 4 @0x0 5 user32.dll wcscpy_s 6 user32.dll DispatchClientMessage 7 xul.dll xul.dll@0x31a0f 8 user32.dll wcscpy_s 9 user32.dll __fnDWORD 10 xul.dll xul.dll@0x31a0f 11 ntdll.dll KiUserCallbackDispatcher 12 ntdll.dll KiUserApcDispatcher 13 xul.dll xul.dll@0x31a0f 14 user32.dll InflateRect 15 user32.dll NtUserDestroyWindow 16 xul.dll nsXULWindow::Destroy xpfe/appshell/src/nsXULWindow.cpp:504 17 xul.dll nsXULWindow::~nsXULWindow xpfe/appshell/src/nsXULWindow.cpp:108 18 xul.dll nsWebShellWindow::`scalar deleting destructor' 19 xul.dll nsWebShellWindow::Release xpfe/appshell/src/nsWebShellWindow.cpp:101 20 xul.dll nsRefPtr<IShellLinkW>::~nsRefPtr<IShellLinkW> 21 xul.dll nsAppShellService::JustCreateTopWindow xpfe/appshell/src/nsAppShellService.cpp:257 22 xul.dll nsAppShellService::CreateHiddenWindow xpfe/appshell/src/nsAppShellService.cpp:105 23 xul.dll nsAppStartup::CreateHiddenWindow toolkit/components/startup/nsAppStartup.cpp:259 24 xul.dll XREMain::XRE_mainRun toolkit/xre/nsAppRunner.cpp:3757 25 xul.dll XREMain::XRE_main toolkit/xre/nsAppRunner.cpp:3890 26 xul.dll XRE_main toolkit/xre/nsAppRunner.cpp:4084 27 firefox.exe wmain toolkit/xre/nsWindowsWMain.cpp:105 28 firefox.exe __tmainCRTStartup crtexe.c:552 29 kernel32.dll BaseThreadInitThunk 30 ntdll.dll __RtlUserThreadStart 31 ntdll.dll _RtlUserThreadStart More reports at: https://crash-stats.mozilla.com/report/list?signature=nsWindow%3A%3ADealWithPopups%28HWND__*%2C+unsigned+int%2C+unsigned+int%2C+long%2C+long*%29

Looks like nsBaseWidget::GetActiveRollupListener(); can return null if nsXULPopupManager::Shutdown() has been called. We should check the result of this call.

Comment on attachment 690831 [details] [diff] [review] check w/some cleanup v.1 One additional cleanup I noticed yesterday is to change the line rollup = rollupListener->ShouldRollupOnMouseWheelEvent(); *outResult = true; to set outResult to MA_ACTIVATE. Maybe we should make a similar crash fix to other platforms?

- fixed up some mixed indentation

Pushed this to try yesterday and got some bustage on B2G which looked unrelated. Pushed it again just to be safe. https://tbpl.mozilla.org/?tree=Try&rev=e5aaf5ebf1ae

Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w Note the -w makes the indentation look a little weird in diff.

Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w > nsWindow::DealWithPopups(HWND inWnd, UINT inMsg, WPARAM inWParam, LPARAM inLParam, LRESULT* outResult) > { > + NS_ASSERTION(outResult, "Bad outResult"); > + > + *outResult = MA_NOACTIVATE; I think it would be clearer if MA_ACTIVATE was the default here, since that's what the normal behaviour would be (in case someone adds a return true elsewhere without setting outResult). > + if (rollup && (inMsg == WM_MOUSEWHEEL || inMsg == WM_MOUSEHWHEEL)) { > rollup = rollupListener->ShouldRollupOnMouseWheelEvent(); > + *outResult = MA_ACTIVATE; > } Actually, after looking at this code some more, I think outResult will always be ignored here anyway as there's never a case where true is returned after this while keeping this assignment to outResult.

This is still the #13 topcrash on Aurora 19. Any plans to land this patch?

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #9) > This is still the #13 topcrash on Aurora 19. Any plans to land this patch? Yes, I'll be back in office on the 31st and will update/land then.

(In reply to Neil Deakin from comment #8) > Comment on attachment 692630 [details] [diff] [review] > big patch v.2 -w > > > nsWindow::DealWithPopups(HWND inWnd, UINT inMsg, WPARAM inWParam, LPARAM inLParam, LRESULT* outResult) > > { > > + NS_ASSERTION(outResult, "Bad outResult"); > > + > > + *outResult = MA_NOACTIVATE; > > I think it would be clearer if MA_ACTIVATE was the default here, since > that's what the normal behaviour would be (in case someone adds a return > true elsewhere without setting outResult). > > > + if (rollup && (inMsg == WM_MOUSEWHEEL || inMsg == WM_MOUSEHWHEEL)) { > > rollup = rollupListener->ShouldRollupOnMouseWheelEvent(); > > + *outResult = MA_ACTIVATE; > > } > > Actually, after looking at this code some more, I think outResult will > always be ignored here anyway as there's never a case where true is returned > after this while keeping this assignment to outResult. We need MA_NOACTIVATE returned here - http://hg.mozilla.org/mozilla-central/annotate/0d771761b9b3/widget/windows/nsWindow.cpp#l8032 http://hg.mozilla.org/mozilla-central/annotate/0d771761b9b3/widget/windows/nsWindow.cpp#l8048 by default. We update it later - http://hg.mozilla.org/mozilla-central/annotate/0d771761b9b3/widget/windows/nsWindow.cpp#l8073 http://hg.mozilla.org/mozilla-central/annotate/0d771761b9b3/widget/windows/nsWindow.cpp#l8092 before returning true. Hence the default. AFAICT the default is set right for all cases where we return true w/out resetting outResult.

Yes, your patch is correct. I commented only to suggest that assigning MA_ACTIVATE by default at the beginning of DealWithPopups was clearer since it's much more likely to be what any new code that might be added in the future would want.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2e77002690 > Yes, your patch is correct. I commented only to suggest that assigning > MA_ACTIVATE by default at the beginning of DealWithPopups was clearer since > it's much more likely to be what any new code that might be added in the > future would want. Ok, well, I had already pushed the exiting patch for a final try run so maybe we can do the change up in a follow up.

I get a lot of warnings spewed to the console after this change, coming from the NS_ENSURE_TRUE(rollupWidget, false); line.

On Windows that was. I see it on Mac too, but not nearly as much.

If those are noisy we can change them to simple if statements. Care to file a follow up and cc me in?

Can we uplift this to Firefox 19 on Beta now that this is resolved?

Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w [Approval Request Comment] Bug caused by (feature/regressing bug #): long term issue User impact if declined: crashiness Testing completed (on m-c, etc.): it has based for a bit, we don't have verification yet though. Risk to taking this patch (and alternatives if risky): this is a sizable change, but is pretty boiler plate work. Mostly added null checks for all platforms and a little reformatting. String or UUID changes made by this patch: none aurora landing will include the follow up fix in bug 826115 to quite a noisy assert.

The patch has already landed in 20.0a2 but not in 19.0 Beta.

(In reply to Jim Mathies [:jimm] from comment #19) > aurora landing will include the follow up fix in bug 826115 to quite a noisy > assert. This already is on Aurora 20, I guess, did you mean to ask for Beta 19 approval instead?

Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w Yeah, sorry. If this isn't a major crash, I'd say we should let the fix ride out on the normal trains.

Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w It's the #19 top (startup) crasher and a regression, so it's highly desirable to take a fix in FF19. Are you concerned that regressions won't be immediately obvious in crash data? What's the worst case scenario?

Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w Spoke with jimm - since there haven't been any reported regressions yet on 19/20, and this remains a top crash, we're comfortable with uplifting to Beta 19 for beta 2 (the sooner the better).

Pushed to try, if these go green I'll push to beta. https://tbpl.mozilla.org/?tree=Try&rev=93bd25184232

This should be fixed on 19b2, but there are still 3 crashes related. Any thoughts?

(In reply to Paul Silaghi [QA] from comment #27) > This should be fixed on 19b2, but there are still 3 crashes related. Any > thoughts? There's no nsXULWindow::Destroy in the stack trace for those three crashes from the same user so it's a different issue that isn't worth a bug for such a low volume.

Agreed. Verified fixed on FF 19 based on comment 28.

There are 4 crashes after the fix on FF 20b1, 20.0a2, but none of them contains nsXULWindow::Destroy in the stack trace. Verified fixed FF 20.