853482 - crash in mozilla::widget::winrt::FrameworkView::UpdateWidgetSizeAndPosition

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect, P1)

Description

[Scoobidiver (away)]

There are two startup crashes from the same user.

Signature 	mozilla::widget::winrt::FrameworkView::UpdateWidgetSizeAndPosition() More Reports Search
UUID	78c2f731-37b6-42ae-8d3f-a38212130321
Date Processed	2013-03-21 12:08:11
Uptime	1
Last Crash	5 seconds before submission
Install Age	21.6 hours since version was first installed.
Install Time	2013-03-20 14:30:46
Product	MetroFirefox
Version	22.0a1
Build ID	20130320031103
Release Channel	nightly
OS	Windows NT
OS Version	6.2.9200
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 58 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
Processor Notes 	sp-processor01.phx1.mozilla.com_27401:2008; WARNING: JSON file missing Add-ons
EMCheckCompatibility	False	
Total Virtual Memory	4294836224
Available Virtual Memory	4156788736
System Memory Use Percentage	51
Available Page File	11404083200
Available Physical Memory	4116754432

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::widget::winrt::FrameworkView::UpdateWidgetSizeAndPosition 	widget/windows/winrt/FrameworkView.cpp:264
1 	xul.dll 	mozilla::widget::winrt::FrameworkView::Run 	widget/windows/winrt/FrameworkView.cpp:114
2 	twinapi.dll 	Windows::ApplicationModel::Core::CoreApplicationView::Run 	d:\win8_gdr\shell\coreapplication\application\lib\coreapplicationview.cpp:888
3 	twinapi.dll 	`Windows::ApplicationModel::Core::CoreApplicationViewAgileContainer::RuntimeClas 	d:\win8_gdr\shell\coreapplication\application\lib\coreapplicationview.cpp:559
4 	twinapi.dll 	`Windows::ApplicationModel::Core::CoreApplicationViewAgileContainer::RuntimeClas 	d:\win8_gdr\shell\coreapplication\application\lib\coreapplicationview.cpp:613
5 	SHCore.dll 	SHReleaseThreadRef 	
6 	kernel32.dll 	BaseThreadInitThunk 	
7 	ntdll.dll 	__RtlUserThreadStart 	
8 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Awidget%3A%3Awinrt%3A%3AFrameworkView%3A%3AUpdateWidgetSizeAndPosition%28%29

Comment 1

[Jim Mathies [:jimm]]

Attachment: fix

Just above this on the stack we are in Run and call Activate on the widget, which can run script. So I'm guessing something goes wrong and the widget gets destroyed. Lets protect against this in release builds but keep the assertions in debug builds for debugging purposes.

Comment 2

[Jim Mathies [:jimm]]

Attachment: fix

Comment 3

[Brian R. Bondy [:bbondy]]

I'm not sure what will happen now if this is called before widget was set though...

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 730102 [details] [diff] [review]
fix

Review of attachment 730102 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/FrameworkView.cpp
@@ +268,3 @@
>    mWidget->Move(0, 0);
>    mWidget->Resize(0, 0, mWindowBounds.width, mWindowBounds.height, true);
>    mWidget->SizeModeChanged();

Should this case be detected and stored in a boolean and then these lines executed if they are hit or something like that?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Comment on attachment 730102 [details] [diff] [review]
> fix
> 
> Review of attachment 730102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/FrameworkView.cpp
> @@ +268,3 @@
> >    mWidget->Move(0, 0);
> >    mWidget->Resize(0, 0, mWindowBounds.width, mWindowBounds.height, true);
> >    mWidget->SizeModeChanged();
> 
> Should this case be detected and stored in a boolean and then these lines
> executed if they are hit or something like that?

Maybe, although we don't get a crash in OnWindowSizeChanged, which interestingly doesn't get set up until a bit later in AddEventHandlers().

Maybe mWindow->Activate() should move down and the initial call to UpdateWidgetSizeAndPosition() should be removed. This would delay killing the splash just a bit. The timing in here has never been 100% reliable.

Comment 6

[Jim Mathies [:jimm]]

We could also block and process events right after mMetroApp->Initialize() until mWidget is valid.

Comment 7

[Brian R. Bondy [:bbondy]]

Maybe just try to skip that code the first time it executes to see what will happen to users in that case.
static bool bFirst = true;
if (!bFirst) {
   mWidget->Move(0, 0);
   mWidget->Resize(0, 0, mWindowBounds.width, mWindowBounds.height, true);
   mWidget->SizeModeChanged();
}
bFirst = false;

Comment 8

[Scoobidiver (away)]

It's #1 top crasher in MetroFirefox 24.0a1.

Comment 9

[Scoobidiver (away)]

It accounts for about 50% of Metro crashes with 6 crashes per 100 ADU.

Comment 11

[Scoobidiver (away)]

Bug 891193 has STR.

Comment 12

[Samvedana (:Samvedana)]

Attachment: Windows error reporting

(In reply to Brian R. Bondy [:bbondy] from comment #3 of bug 891193)
> Please also check the windows event log and paste the error that appears
> there here. Thanks!

I was able to reproduce this issue since yesterday. I went through log and found some information as attached here (3 attachments which are logged at the same time).

Comment 13

[Samvedana (:Samvedana)]

Attachment: apps

Comment 14

[Samvedana (:Samvedana)]

Attachment: applications hang

Comment 15

[Jim Mathies [:jimm]]

Attachment: tmp.txt

Taking a shot at addressing this. Not sure why the widget is so late in getting created, might be a sign of something else wrong. But hopefully we can address this top crash by waiting for it.

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 792817 [details] [diff] [review]
tmp.txt

oops, that had some misc. debug gunk in it.

Comment 17

[Jim Mathies [:jimm]]

Attachment: patch

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 792819 [details] [diff] [review]
patch

Review of attachment 792819 [details] [diff] [review]:
-----------------------------------------------------------------

I'm wary to add so much complexity (an event and a new place where we spin an event loop) that we intend to rip out once we've solved the real issue here (the real issue being that the lifecycles of the widget, window, and framework view need to be better co-managed).

Can we solve this issue with some well-placed 'if' checks and by calling UpdateWidgetSizeAndPosition from SetWindow and SetWidget?

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Something like this

I haven't built or tested this, but I think something like this might be a cleaner way to work around the issues we're seeing

Comment 20

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #18)
> Comment on attachment 792819 [details] [diff] [review]
> patch
> 
> Review of attachment 792819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm wary to add so much complexity (an event and a new place where we spin
> an event loop) that we intend to rip out once we've solved the real issue
> here (the real issue being that the lifecycles of the widget, window, and
> framework view need to be better co-managed).

The whole point of this patch to to solve the issue. /me scratches head

> Can we solve this issue with some well-placed 'if' checks and by calling
> UpdateWidgetSizeAndPosition from SetWindow and SetWidget?

We need to wait until xpcom finishes initializing and the widget is created. Everything is pretty dependent on a valid widget since it's the main interface between us and the screen.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 792910 [details] [diff] [review]
Something like this

Review of attachment 792910 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/FrameworkView.cpp
@@ +274,5 @@
> +  // crashes (see bug 853482). When the real cause of this issue has been
> +  // addressed, we can remove this 'if' check.
> +  if (mWidget && mWindow) {
> +    mWidget->Move(0, 0);
> +    mWidget->Resize(0, 0, mWindowBounds.width, mWindowBounds.height, true);

We need this code to execute reliably, otherwise widget doesn't have dims.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

> > I'm wary to add so much complexity (an event and a new place where we spin
> > an event loop) that we intend to rip out once we've solved the real issue
> > here (the real issue being that the lifecycles of the widget, window, and
> > framework view need to be better co-managed).
> 
> The whole point of this patch to to solve the issue. /me scratches head

Oh, I thought from what you said in comment 15 that this was a temporary solution.

This patch will definitely fix the crash, and it also helps fix the "real issue" described above by making the FrameworkView wait until it has had SetWidget called, but it feels like we should be able to use a simpler mechanism to guarantee that our FrameworkView is always in a consistent state. Since the FrameworkView really can't do anything without a widget and a window, maybe it should take those as arguments to its constructor?

If it turns out that it's a lot of extra effort to simplify the interactions between the widget, framework view, and window, then I'm OK with adding logic to the FrameworkView to wait for the widget to be ready, for now. If we go this route, let's also file a follow-up bug for refactoring these interactions.