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

RESOLVED FIXED in Firefox 47

Status

()

P1
normal
Rank:
10
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: jimm)

Tracking

({crash, regression})

Trunk
mozilla49
crash, regression
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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)

Comment 1

2 years ago
Jim, could you look at this? Thanks.
Flags: needinfo?(jmathies)
(Reporter)

Updated

2 years ago
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
(Assignee)

Comment 3

2 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

2 years ago
Oh, duh, |ok| might be false without the rv.
(Assignee)

Comment 5

2 years ago
Created attachment 8747175 [details] [diff] [review]
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.
Assignee: nobody → jmathies
Attachment #8747175 - Flags: review?(continuation)
(Reporter)

Comment 6

2 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

2 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

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9ed1f5f5358
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 12

2 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 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.
status-firefox48: --- → affected

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab4a17a4d9d6
status-firefox48: affected → fixed
(Assignee)

Comment 16

2 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+

Updated

2 years ago
status-firefox47: --- → affected

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4f2def663d84
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.