819888 - crash in nsWindow::DealWithPopups

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Scoobidiver (away)]

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

Comment 1

[Jim Mathies [:jimm]]

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

Comment 2

[Jim Mathies [:jimm]]

Attachment: check w/some cleanup v.1

Comment 3

[Neil Deakin]

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?

Comment 4

[Jim Mathies [:jimm]]

Attachment: big patch v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: big patch v.2

- fixed up some mixed indentation

Comment 6

[Jim Mathies [:jimm]]

Attachment: 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

Comment 7

[Jim Mathies [:jimm]]

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 8

[Neil Deakin]

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.

Comment 9

[Robert Kaiser]

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

Comment 10

[Jim Mathies [:jimm]]

(In reply to Robert Kaiser (:[e-mail]) 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.

Comment 11

[Jim Mathies [:jimm]]

(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

[Neil Deakin]

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.

Comment 13

[Jim Mathies [:jimm]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a2e77002690

Comment 15

[Cameron McCormack (:heycam)]

I get a lot of warnings spewed to the console after this change, coming from the

  NS_ENSURE_TRUE(rollupWidget, false);

line.

Comment 16

[Cameron McCormack (:heycam)]

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

Comment 17

[Jim Mathies [:jimm]]

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

Comment 18

[Alex Keybl [:akeybl]]

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

Comment 19

[Jim Mathies [:jimm]]

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.

Comment 20

[Scoobidiver (away)]

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

Comment 21

[Robert Kaiser]

(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 22

[Jim Mathies [:jimm]]

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 23

[Alex Keybl [:akeybl]]

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

[Alex Keybl [:akeybl]]

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).

Comment 25

[Jim Mathies [:jimm]]

Pushed to try, if these go green I'll push to beta.

https://tbpl.mozilla.org/?tree=Try&rev=93bd25184232

Comment 26

[Jim Mathies [:jimm]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e4d605639e1d

Comment 27

[Paul Silaghi, QA [:pauly]]

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

Comment 28

[Scoobidiver (away)]

(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

[Paul Silaghi, QA [:pauly]]

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

Comment 30

[Paul Silaghi, QA [:pauly]]

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.