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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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).

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Jim, could you look at this? Thanks.

Comment 2

[Maire Reavy [:mreavy]]

Jim -- If you're swamped, Anthony and I can try to find someone else.  (FYI: cpearce is on PTO until May 4th.)

Comment 3

[Jim Mathies [:jimm]]

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.

Comment 4

[Jim Mathies [:jimm]]

Oh, duh, |ok| might be false without the rv.

Comment 5

[Jim Mathies [:jimm]]

Attachment: fix

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.

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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.

Comment 7

[Jim Mathies [:jimm]]

(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.

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ed1f5f5358

Comment 10

[Jim Mathies [:jimm]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9ed1f5f5358

Comment 12

[Jim Mathies [:jimm]]

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

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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

e10s crash fix that should land along with the changes in bug 1263951

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Crash-stats doesn't show any reports with this signature for 48, but marking it affected anyway based on jimm's request.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab4a17a4d9d6

Comment 16

[Jim Mathies [:jimm]]

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

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

e10s crash fix, Beta47+

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f2def663d84