crash in nsWindow::DealWithPopups

VERIFIED FIXED in Firefox 19

Status

()

--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: scoobidiver, Assigned: jimm)

Tracking

({crash, regression, topcrash})

19 Branch
mozilla20
x86
Windows XP
crash, regression, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19+ verified, firefox20 verified)

Details

(Whiteboard: [startupcrash], crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
tracking-firefox19: --- → ?
(Reporter)

Updated

6 years ago
status-firefox19: --- → affected
status-firefox20: --- → affected

Updated

6 years ago
Assignee: nobody → jmathies
tracking-firefox19: ? → +
(Assignee)

Comment 1

6 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

6 years ago
Created attachment 690831 [details] [diff] [review]
check w/some cleanup v.1
Attachment #690831 - Flags: review?(enndeakin)

Comment 3

6 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

6 years ago
Created attachment 692410 [details] [diff] [review]
big patch v.1
(Assignee)

Comment 5

6 years ago
Created attachment 692413 [details] [diff] [review]
big patch v.2

- fixed up some mixed indentation
Attachment #692410 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #692413 - Attachment is patch: true
(Assignee)

Comment 6

6 years ago
Created attachment 692630 [details] [diff] [review]
big patch v.2 -w

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

6 years ago
Attachment #690831 - Attachment is obsolete: true
(Assignee)

Comment 7

6 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

6 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

6 years ago
This is still the #13 topcrash on Aurora 19. Any plans to land this patch?
Flags: needinfo?(jmathies)
(Assignee)

Comment 10

6 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

6 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

6 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

6 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.
https://hg.mozilla.org/mozilla-central/rev/1a2e77002690
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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.
(Assignee)

Comment 17

6 years ago
If those are noisy we can change them to simple if statements. Care to file a follow up and cc me in?
Depends on: 826115
(Reporter)

Updated

6 years ago
status-firefox20: affected → fixed
Can we uplift this to Firefox 19 on Beta now that this is resolved?
(Assignee)

Comment 19

6 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

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

Comment 21

6 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

6 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 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).
Attachment #692630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 25

6 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

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/e4d605639e1d
status-firefox19: affected → fixed

Updated

6 years ago
Depends on: 831342
This should be fixed on 19b2, but there are still 3 crashes related. Any thoughts?
(Reporter)

Comment 28

6 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.
Agreed. Verified fixed on FF 19 based on comment 28.
status-firefox19: fixed → verified
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.
Status: RESOLVED → VERIFIED
status-firefox20: fixed → verified
You need to log in before you can comment on or make changes to this bug.