Closed
Bug 1268714
Opened 8 years ago
Closed 8 years ago
Near-null crash in gmp::GetContentParentFromDone::Done
Categories
(Core :: Audio/Video: GMP, defect, P1)
Core
Audio/Video: GMP
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mccr8, Assigned: jimm)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
1.24 KB,
patch
|
mccr8
:
review+
lizzard
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
It looks like maybe parent is null? This could be a regression from bug 1263951. This is the number 8 crash on the 4/27 Nightly (with 10 crashes).
Reporter | ||
Updated•8 years ago
|
Keywords: crash,
regression
Comment 2•8 years ago
|
||
Jim -- If you're swamped, Anthony and I can try to find someone else. (FYI: cpearce is on PTO until May 4th.)
Rank: 10
Priority: -- → P1
Assignee | ||
Comment 3•8 years ago
|
||
Hmm, odd. The failure result I returned is picked up just above this line so I'm sure how I could have regressed it. Let me poke a bit.
Flags: needinfo?(jmathies)
Assignee | ||
Comment 4•8 years ago
|
||
Oh, duh, |ok| might be false without the rv.
Assignee | ||
Comment 5•8 years ago
|
||
Total brain fart on my part. The original code checked for false return from the IPC call which is typical. For some reason I added the |&& rv == errorval| vs or'ing and checking rv independently.
Assignee: nobody → jmathies
Attachment #8747175 -
Flags: review?(continuation)
Reporter | ||
Comment 6•8 years ago
|
||
Comment on attachment 8747175 [details] [diff] [review] fix Review of attachment 8747175 [details] [diff] [review]: ----------------------------------------------------------------- Seems kind of weird to return true and an error but this check does fix the issue.
Attachment #8747175 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #6) > Comment on attachment 8747175 [details] [diff] [review] > fix > > Review of attachment 8747175 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems kind of weird to return true and an error but this check does fix the > issue. This is the way we've been handling IPC error that we want to propagate back and handle in the child. If we return false on the parent side it triggers a abort.
Reporter | ||
Comment 8•8 years ago
|
||
That makes sense. Maybe it should be |NS_FAILED(rv)| instead of |rv == NS_ERROR_ILLEGAL_DURING_SHUTDOWN| in case there are other ways to fail in the future.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #8) > That makes sense. Maybe it should be |NS_FAILED(rv)| instead of |rv == > NS_ERROR_ILLEGAL_DURING_SHUTDOWN| in case there are other ways to fail in > the future. Too late for this change. I thought about doing it this way, I was afraid someone might populate aRv without realizing the side effects. Next person to mess with it can update the check.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9ed1f5f5358
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8747175 [details] [diff] [review] fix Approval Request Comment [Feature/regressing bug #]: followup fix for e10s crash bug 1263951 [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 #8747175 -
Flags: approval-mozilla-aurora?
Comment 13•8 years ago
|
||
Comment on attachment 8747175 [details] [diff] [review] fix e10s crash fix that should land along with the changes in bug 1263951
Attachment #8747175 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•8 years ago
|
||
Crash-stats doesn't show any reports with this signature for 48, but marking it affected anyway based on jimm's request.
status-firefox48:
--- → affected
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab4a17a4d9d6
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8747175 [details] [diff] [review] fix Approval Request Comment [Feature/regressing bug #]: followup fix for e10s crash bug 1263951 [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]: low [String/UUID change made/needed]: none
Attachment #8747175 -
Flags: approval-mozilla-beta?
Comment on attachment 8747175 [details] [diff] [review] fix e10s crash fix, Beta47+
Attachment #8747175 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox47:
--- → affected
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4f2def663d84
You need to log in
before you can comment on or make changes to this bug.
Description
•