1150916 - [Crash][@ nsQueryInterface::operator() | nsCOMPtr_base::assign_from_qi]

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Stability, defect)

Description

[Nicholas Troast [:ntroast]]

We observed the following crash signature during testing.

[@ nsQueryInterface::operator() | nsCOMPtr_base::assign_from_qi | xpc_TryUnmarkWrappedGrayObject | nsObserverList::UnmarkGrayStrongObservers ]

Cafbot will upload the decoded minidump and extra file.

Comment 1

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 2

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 3

[bhavana bajaj [:bajaj]]

NI Birunthan/Froyd, to start with as they seem to have touched this area of code or can help direct to right people.

Please note, this is a critical FXOS reqression impacting our critical release milestone and any help is appreciated.

Comment 4

[Nathan Froyd [:froydnj]]

Uh.  Looks like we have a bogus nsIObserver pointer while we're notifying observers during a CC while we're minimizing memory?  ni?'ing mccr8 to see if the CC stack looks like something recognizable.

Comment 5

[Andrew McCreight [:mccr8]]

I haven't seen anything like this in particular before.  It looks like we're QIing something and it crashes in there?  The crash address, 0x1000000, is odd.  Could the nsIObserver pointer be null?  I'd hope we'd be safe from totally bogus pointers, but maybe not.

poiru just did style clean up for this file.

Comment 6

[Nicholas Troast [:ntroast]]

Looks like all the ni? got cleared here. Bhavana, based on the information provided so far is there anyone else we can loop in here?

Comment 7

[Andrew McCreight [:mccr8]]

Nathan and I are looking into it.

Comment 8

[Andrew McCreight [:mccr8]]

Is this a crash that happened a single time, or one that has come up more than once?

Comment 9

[Nicholas Troast [:ntroast]]

This crash came up 4 times during the test run.

Comment 10

[Nathan Froyd [:froydnj]]

Exact same crash report (registers, crash address, etc.) each time?

Comment 11

[Nicholas Troast [:ntroast]]

I can have cafbot upload all the decoded minidumps.

Comment 12

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 13

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 14

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 15

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 16

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 17

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 18

[Andrew McCreight [:mccr8]]

Thanks.  Yes, all of the crashes are on the same address, 0x1000000.

Comment 19

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.119
Moz BuildID: 20150330002503
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.119.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=473cd63f53c855299b719285d9b95e3f2910782f
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=e44e28d2a37258fe0c908c59f2e91e5287777b40
Patches: bug 1133398, bug 1143694, bug 1146987, bug 1145724, bug 1147646, bug 1133147, bug 1150271, bug 1142770

Comment 20

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.119

Comment 21

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.119

Comment 22

[Andrew McCreight [:mccr8]]

I've been reading through this code but I haven't come up with anything in particular.

The best idea I've had is to write some code that looks for this value we keep crashing on, and print out the particular observer message thing we're looking for if it finds it, then crashes.  If we managed to hit that code, it would give us a better idea where things are going wrong, and, if it is related to the actual observer, a smaller amount of code to look at.

Comment 23

[Inder]

Do you want to provide a logging patch to identify the issue?

Comment 24

[Andrew McCreight [:mccr8]]

I'll work on a logging patch.

Benjamin, do you have any ideas?  As perhaps the person most familiar with nsObserverService.

Comment 25

[Benjamin Smedberg]

Crash reason:  SIGSEGV
Crash address: 0x1000000

That's a totally suspicious crash address (it's the content of r0).

Not knowing much about b2g release engineering, do we have the disassembly of nsQueryInterface::operator() so that we know what is supposed to be in r0 at the time of this crash? And do we have a core which will show us whether 0x1000000 is a reasonable or crazy address?

I am automatically suspicious here also because this is inside of a hashtable enumeration: if anyone is modifying the observer hashtable from within this stack, then the enumeration is bad:

nsObserverList::UnmarkGrayStrongObservers
-> xpc_TryUnmarkWrappedGrayObject

Comment 26

[Andrew McCreight [:mccr8]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> I am automatically suspicious here also because this is inside of a
> hashtable enumeration: if anyone is modifying the observer hashtable from
> within this stack, then the enumeration is bad:
> 
> nsObserverList::UnmarkGrayStrongObservers
> -> xpc_TryUnmarkWrappedGrayObject

Yeah, I suppose I should do address that before I go on more of a fishing expedition.  xpc_TryUnmarkWrappedGrayObject does a QI then an UnmarkGrayGCThing.  The latter is a JS engine thing that can't GC according to the static analysis, so that should be okay, but a QI could do anything, including triggering a GC, so that would be bad.

Comment 27

[Andrew McCreight [:mccr8]]

Attachment: Be sure we aren't iterating over a hashtable while it is modified in nsObserverService::UnmarkGrayStrongObservers().

I have only tested this a little bit locally, but maybe this will help, if you want to try it out.

This copies out all strong observers into a single array, then does the unmarking iteration over that. I could certainly see a QI for a JS-implemented thing somehow triggering a GC and causing observer problems, though I wasn't able to trigger that locally.

We'll see how the try run goes:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=85ff1816a341

Comment 28

[Andrew McCreight [:mccr8]]

The try run looks fine.  You could see if this patch fixes the crash.

Comment 29

[Inder]

Thanks.
Nick/Greg - can you please land it in our build.

Comment 30

[Andrew McCreight [:mccr8]]

Attachment: Be sure we aren't iterating over a hashtable while it is modified in nsObserverService::UnmarkGrayStrongObservers().

I just renamed a function and removed a comment.  This probably makes sense to land even if it doesn't fix this particular issue.

Comment 31

[Greg Grisco]

(In reply to Inder from comment #29)
> Thanks.
> Nick/Greg - can you please land it in our build.

Ok, working on it.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8588813 [details] [diff] [review]
Be sure we aren't iterating over a hashtable while it is modified in nsObserverService::UnmarkGrayStrongObservers().

Review of attachment 8588813 [details] [diff] [review]:
-----------------------------------------------------------------

I think making the commit comment a little more explicit, something like:

"It may be the case that the QI in xpc_TryUnmarkWrappedGrayObject could cause a GC, which might cause an observer to be destroyed, which might removeObserver() itself, which might mutate the hashtable that we're attempting to enumerate."

would be good.  I didn't fully grok what was going on until mccr8 made it clear to me on IRC.

Comment 33

[Andrew McCreight [:mccr8]]

Updated the comment to be more explicit.  Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/074e4fb089b1

Comment 34

[Keven Kuo [:kkuo]]

Comment on attachment 8588813 [details] [diff] [review]
Be sure we aren't iterating over a hashtable while it is modified in nsObserverService::UnmarkGrayStrongObservers().

[Triage Comment]

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/074e4fb089b1

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/78f1fe03e1d4

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Follow-up fix for non-unified bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4dd555969f3
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1299110a2546

Comment 38

[Carsten Book [:Tomcat]]

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

Comment 39

[Keven Kuo [:kkuo]]

Since this is landed on 2.2.
Mark the status as FIXED.

--
Keven