Closed Bug 1268714 Opened 4 years ago Closed 4 years ago

Near-null crash in gmp::GetContentParentFromDone::Done

Categories

(Core :: Audio/Video: GMP, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mccr8, Assigned: jimm)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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).
Jim, could you look at this? Thanks.
Flags: needinfo?(jmathies)
Keywords: crash, regression
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
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)
Oh, duh, |ok| might be false without the rv.
Attached patch fixSplinter Review
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)
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+
(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.
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/c9ed1f5f5358
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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 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+
Crash-stats doesn't show any reports with this signature for 48, but marking it affected anyway based on jimm's request.
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+
You need to log in before you can comment on or make changes to this bug.