Closed
Bug 853482
Opened 12 years ago
Closed 11 years ago
crash in mozilla::widget::winrt::FrameworkView::UpdateWidgetSizeAndPosition
Categories
(Core Graveyard :: Widget: WinRT, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla26
People
(Reporter: scoobidiver, Assigned: jimm)
References
Details
(Keywords: crash, reproducible, Whiteboard: [metro-crash][startupcrash])
Crash Data
Attachments
(8 obsolete files)
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
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee: nobody → jmathies
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #730100 -
Attachment is obsolete: true
Attachment #730102 -
Flags: review?(netzen)
Updated•12 years ago
|
Attachment #730102 -
Flags: review?(netzen) → review+
Comment 3•12 years ago
|
||
I'm not sure what will happen now if this is called before widget was set though...
Comment 4•12 years ago
|
||
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?
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Comment 6•12 years ago
|
||
We could also block and process events right after mMetroApp->Initialize() until mWidget is valid.
Comment 7•12 years ago
|
||
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;
Reporter | ||
Updated•12 years ago
|
Hardware: x86 → All
Whiteboard: [metro-crash] → [metro-crash][startupcrash]
Reporter | ||
Comment 8•12 years ago
|
||
It's #1 top crasher in MetroFirefox 24.0a1.
Reporter | ||
Comment 9•12 years ago
|
||
It accounts for about 50% of Metro crashes with 6 crashes per 100 ADU.
Assignee | ||
Updated•12 years ago
|
Priority: -- → P1
Comment 12•11 years ago
|
||
(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•11 years ago
|
||
Comment 14•11 years ago
|
||
Assignee | ||
Comment 15•11 years ago
|
||
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.
Attachment #730102 -
Attachment is obsolete: true
Attachment #772978 -
Attachment is obsolete: true
Attachment #772980 -
Attachment is obsolete: true
Attachment #772981 -
Attachment is obsolete: true
Attachment #792817 -
Flags: review?(tabraldes)
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 792817 [details] [diff] [review]
tmp.txt
oops, that had some misc. debug gunk in it.
Attachment #792817 -
Flags: review?(tabraldes)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #792817 -
Attachment is obsolete: true
Attachment #792819 -
Flags: review?(tabraldes)
Comment 18•11 years ago
|
||
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?
Attachment #792819 -
Flags: review?(tabraldes)
Comment 19•11 years ago
|
||
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
Assignee | ||
Comment 20•11 years ago
|
||
(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.
Assignee | ||
Comment 21•11 years ago
|
||
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•11 years ago
|
||
> > 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.
Assignee | ||
Updated•11 years ago
|
Attachment #792819 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #792910 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Assignee: nobody → jmathies
Target Milestone: --- → mozilla26
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•