Closed
Bug 819888
Opened 12 years ago
Closed 12 years ago
crash in nsWindow::DealWithPopups
Categories
(Core :: Widget: Win32, defect)
Tracking
()
VERIFIED
FIXED
mozilla20
People
(Reporter: scoobidiver, Assigned: jimm)
References
Details
(Keywords: crash, regression, topcrash, Whiteboard: [startupcrash])
Crash Data
Attachments
(1 file, 3 obsolete files)
21.22 KB,
patch
|
enndeakin
:
review+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•12 years ago
|
tracking-firefox19:
--- → ?
Reporter | ||
Updated•12 years ago
|
status-firefox19:
--- → affected
status-firefox20:
--- → affected
Updated•12 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Comment 1•12 years ago
|
||
Looks like nsBaseWidget::GetActiveRollupListener(); can return null if nsXULPopupManager::Shutdown() has been called. We should check the result of this call.
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #690831 -
Flags: review?(enndeakin)
Comment 3•12 years ago
|
||
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?
Attachment #690831 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
- fixed up some mixed indentation
Attachment #692410 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #692413 -
Attachment is patch: true
Assignee | ||
Comment 6•12 years ago
|
||
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
Attachment #692413 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #690831 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 692630 [details] [diff] [review] big patch v.2 -w Note the -w makes the indentation look a little weird in diff.
Attachment #692630 -
Flags: review?(enndeakin)
Comment 8•12 years ago
|
||
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.
Attachment #692630 -
Flags: review?(enndeakin) → review+
Comment 9•12 years ago
|
||
This is still the #13 topcrash on Aurora 19. Any plans to land this patch?
Flags: needinfo?(jmathies)
Assignee | ||
Comment 10•12 years ago
|
||
(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.
Flags: needinfo?(jmathies)
Assignee | ||
Comment 11•12 years ago
|
||
(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.
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
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.
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a2e77002690
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 15•12 years ago
|
||
I get a lot of warnings spewed to the console after this change, coming from the NS_ENSURE_TRUE(rollupWidget, false); line.
Comment 16•12 years ago
|
||
On Windows that was. I see it on Mac too, but not nearly as much.
Assignee | ||
Comment 17•12 years ago
|
||
If those are noisy we can change them to simple if statements. Care to file a follow up and cc me in?
Reporter | ||
Updated•12 years ago
|
Comment 18•12 years ago
|
||
Can we uplift this to Firefox 19 on Beta now that this is resolved?
Assignee | ||
Comment 19•12 years ago
|
||
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.
Attachment #692630 -
Flags: approval-mozilla-aurora?
Reporter | ||
Comment 20•12 years ago
|
||
The patch has already landed in 20.0a2 but not in 19.0 Beta.
Comment 21•11 years ago
|
||
(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?
Assignee | ||
Comment 22•11 years ago
|
||
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.
Attachment #692630 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment 23•11 years ago
|
||
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 24•11 years ago
|
||
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).
Attachment #692630 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 25•11 years ago
|
||
Pushed to try, if these go green I'll push to beta. https://tbpl.mozilla.org/?tree=Try&rev=93bd25184232
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/e4d605639e1d
Comment 27•11 years ago
|
||
This should be fixed on 19b2, but there are still 3 crashes related. Any thoughts?
Reporter | ||
Comment 28•11 years ago
|
||
(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.
Comment 29•11 years ago
|
||
Agreed. Verified fixed on FF 19 based on comment 28.
Comment 30•11 years ago
|
||
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.
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•