Closed
Bug 1263951
Opened 9 years ago
Closed 9 years ago
crash in mozilla::ipc::TransferHandleToProcess (GMP related)
Categories
(Core :: Audio/Video: GMP, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: jimm, Assigned: jimm)
References
Details
(Keywords: crash)
Crash Data
Attachments
(2 files, 1 obsolete file)
5.55 KB,
patch
|
jimm
:
review+
lizzard
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.58 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1258317 +++
This signature breaks down into two sub groups, this bug tracks GMP related issues. In particular 1.a, which should be pretty easy to track down.
1) Parent side crashes during GMP init on a background thread, made up of two sub groups:
a) 55 (18%) Crashes that happen during a shutdown race where the main thread is tearing down the GMP child process at the same time the child is initializing.
b) 38 (12%) A generic TransferHandleToProcess, mostly associated with GMP.
2) 244 (~66%) Failures to transfer a handle in the child, mostly during the init of PBackground::Open. (We don't have GetLastError data for these yet.)
Assignee | ||
Comment 1•9 years ago
|
||
Maire, is this something your team might be able to pick up?
Flags: needinfo?(mreavy)
Assignee | ||
Updated•9 years ago
|
Summary: crash in mozilla::ipc::TransferHandleToProcess → crash in mozilla::ipc::TransferHandleToProcess (GMP related)
Comment 2•9 years ago
|
||
Chris -- Can you (or someone you designate) take a look at this and identify where the bug is and what needs to get fixed? The fact that we are init'ing GMP during Shutdown sounds problematic. Thanks.
Flags: needinfo?(mreavy) → needinfo?(cpearce)
Updated•9 years ago
|
Rank: 10
Comment 3•9 years ago
|
||
jimm: How did you conclude that the 1a crashes here were caused by shutdown? For example, this parent crash: https://crash-stats.mozilla.com/report/index/599a2ee5-43ff-49ae-b3fb-dfa452160416#allthreads doesn't have anything on the main thread call stack to indicate that shutdown has begun.
Flags: needinfo?(jmathies)
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #3)
> jimm: How did you conclude that the 1a crashes here were caused by shutdown?
> For example, this parent crash:
> https://crash-stats.mozilla.com/report/index/599a2ee5-43ff-49ae-b3fb-
> dfa452160416#allthreads doesn't have anything on the main thread call stack
> to indicate that shutdown has begun.
I was looking at crashes that have ShutdownProgress set.
https://crash-stats.mozilla.com/search/?product=Firefox&signature=~TransferHandleToProcess&_facets=signature&_facets=shutdown_progress&_facets=system_error&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports
In this search I see 87 crashes, of which 26 have something set in ShutdownProgress (profile-change-teardown), most (95%?) have a crashing background thread initializing GMP.
https://crash-stats.mozilla.com/search/?product=Firefox&signature=~TransferHandleToProcess&shutdown_progress=change&_facets=signature&_facets=shutdown_progress&_facets=system_error&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports
Flags: needinfo?(jmathies)
Assignee | ||
Comment 5•9 years ago
|
||
Sounds like GMP shutdown can take a long time, which might explain how we can get caught trying to init gmp during shut down.
http://mxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPServiceParent.cpp#363
Should we check if we're shutting down in GMPServiceParent::RecvLoadGMP through mService? That points to GeckoMediaPluginServiceParent which has a variable mWaitingForPluginsSyncShutdown that tracks observer shutdown notifications. Then we just need to be sure GMP code handles a failure in RecvLoadGMP gracefully.
This might not catch all these failures, but it should help substantially assuming that shutdown execution in the observer takes "a long time".
Assignee | ||
Comment 6•9 years ago
|
||
This needs to be tested.
Attachment #8742892 -
Flags: feedback?(cpearce)
Assignee | ||
Comment 7•9 years ago
|
||
Try push with browser chrome looks good -
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20c64f61dfcb18f3b9e7efd571c3b5289094adc4
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cpearce)
Comment 8•9 years ago
|
||
Comment on attachment 8742892 [details] [diff] [review]
wip
Review of attachment 8742892 [details] [diff] [review]:
-----------------------------------------------------------------
Seems reasonable to me.
Attachment #8742892 -
Flags: feedback?(cpearce) → feedback+
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8742892 [details] [diff] [review]
wip
ok :), can I get an r+ on it?
Attachment #8742892 -
Flags: review?(cpearce)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Updated•9 years ago
|
Blocks: e10s-crashes
No longer depends on: 1258317
Comment 10•9 years ago
|
||
Comment on attachment 8742892 [details] [diff] [review]
wip
Review of attachment 8742892 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/gmp/GMPServiceParent.cpp
@@ +690,5 @@
> +{
> + MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> + return mShuttingDownOnGMPThread;
> +}
> +
Nit: Extra line break here.
::: layout/generic/test/mochitest.ini
@@ +115,5 @@
> [test_overflow_event.html]
> [test_page_scroll_with_fixed_pos.html]
> skip-if = buildapp == 'mulet' || (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) # b2g-debug(opened window too small?) b2g-desktop(opened window too small?)
> support-files = page_scroll_with_fixed_pos_window.html
> [test_plugin_clipping.xhtml]
These tests look like they're NPAPI plugin tests, and not GMP related. Seems to me that the change to re-enable these tests should be in a different bug, since it seems unlikely (or at lest unbelievable) that the change you're making would affect these tests.
Attachment #8742892 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 11•9 years ago
|
||
> ::: layout/generic/test/mochitest.ini
> @@ +115,5 @@
> > [test_overflow_event.html]
> > [test_page_scroll_with_fixed_pos.html]
> > skip-if = buildapp == 'mulet' || (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) # b2g-debug(opened window too small?) b2g-desktop(opened window too small?)
> > support-files = page_scroll_with_fixed_pos_window.html
> > [test_plugin_clipping.xhtml]
>
> These tests look like they're NPAPI plugin tests, and not GMP related. Seems
> to me that the change to re-enable these tests should be in a different bug,
> since it seems unlikely (or at lest unbelievable) that the change you're
> making would affect these tests.
Sorry about that, this leaked through from some other e10s work in my queue.
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8742892 -
Attachment is obsolete: true
Attachment #8745431 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Keywords: checkin-needed
Comment 14•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•9 years ago
|
Target Milestone: mozilla48 → mozilla49
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8745431 [details] [diff] [review]
patch
Approval Request Comment
[Feature/regressing bug #]:
e10s related crash fix (need to include the follow up in bug 126874)
[User impact if declined]:
more crashy e10s
[Describe test coverage new/current, TreeHerder]:
it's been on nightly now since 4-27
[Risks and why]:
modest
[String/UUID change made/needed]:
none
Attachment #8745431 -
Flags: approval-mozilla-aurora?
Comment 16•9 years ago
|
||
Marking affected for 47 and 48 based on crash-stats
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Comment 17•9 years ago
|
||
Comment on attachment 8745431 [details] [diff] [review]
patch
Less crashy e10s sounds good, please uplift to aurora.
Attachment #8745431 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•9 years ago
|
||
Note, followup issue is in bug 1268714
Comment 19•9 years ago
|
||
bugherder uplift |
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 20•9 years ago
|
||
shoot, sorry. I was thinking of another bug similar to this one.
Hi Jim, do we need this on Beta47? Just wondering.
Flags: needinfo?(jmathies)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8745431 [details] [diff] [review]
patch
Approval Request Comment
[Feature/regressing bug #]:
e10s related crash fix (need to include the follow up in bug 126874)
[User impact if declined]:
more crashy e10s
[Describe test coverage new/current, TreeHerder]:
it's been on nightly and aurora for a while
[Risks and why]:
low
[String/UUID change made/needed]:
none
Flags: needinfo?(jmathies)
Attachment #8745431 -
Flags: approval-mozilla-beta?
Comment on attachment 8745431 [details] [diff] [review]
patch
I looked at the data on Nightly and Aurora and we seem to be hitting this crash still. I didn't dig into the data more to see if the trends shows a reduction in crash volume since the fix. Still seems reasonable to uplift, Beta47+
Attachment #8745431 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #23)
> Comment on attachment 8745431 [details] [diff] [review]
> patch
>
> I looked at the data on Nightly and Aurora and we seem to be hitting this
> crash still. I didn't dig into the data more to see if the trends shows a
> reduction in crash volume since the fix. Still seems reasonable to uplift,
> Beta47+
This is a two parter, bug 1258317 covers another class of crashes with the same signature. I checked nightly and found one crash related to gmp that snuck through before we entered shutdown. This was a substantial improvement compared to prior to this work landing. Hence why I marked it for uplift. bug 1271601 also help with any remaining issues, although we're not blocking on that work.
Comment 25•9 years ago
|
||
has problems uplifting to beta:
grafting 343679:61a1435b8d61 "Bug 1263951 - Avoid trying to initialize new GMP instances once the browser enters shutdown. r=cpearce a=lizzard"
merging dom/media/gmp/GMPServiceParent.cpp
merging dom/media/gmp/GMPServiceParent.h
warning: conflicts while merging dom/media/gmp/GMPServiceParent.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jmathies)
Assignee | ||
Comment 26•9 years ago
|
||
fixes a minor merge conflict, nothing major.
Flags: needinfo?(jmathies)
Comment 27•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•