615458 - Fennec 4.0b3pre Crash Report [@ mozilla::dom::ContentParent::Observe ]

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[(no longer active)]

Here are my crash stacks:

http://crash-stats.mozilla.com/report/index/bp-ba92e805-0d26-4425-9441-a01502101128
http://crash-stats.mozilla.com/report/index/bp-5952c36c-6c3c-4374-8a39-978712101128

Comment 1

[Doug Turner (:dougt)]

your build was from the 25th.  I fixed this problem in bug 614801.

Also see: http://hg.mozilla.org/mozilla-central/rev/eaf1cd30f172

Do you have other crash stacks from a newer build?

Comment 2

[(no longer active)]

(In reply to comment #1)
> your build was from the 25th.  I fixed this problem in bug 614801.
> 
> Also see: http://hg.mozilla.org/mozilla-central/rev/eaf1cd30f172

Hmm, the landscape/portrait rotation crash has been fixed in my build...

> Do you have other crash stacks from a newer build?

No, these are my last two crashes.

Comment 4

[(no longer active)]

Let's not dupe this against bug 614801 until we're sure that the crash is really fixed (it doesn't seem to be, AFAICT).

Comment 5

[Scoobidiver (away)]

Here is a link to all Fennec reports:
http://crash-stats.mozilla.com/report/list?product=Fennec&query_search=signature&range_value=4&range_unit=weeks&signature=mozilla%3A%3Adom%3A%3AContentParent%3A%3AObserve
There are no crashes with this signature from 4.0b3pre/20101126 build, that is 4 consecutive builds without crashes.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Here is a link to all Fennec reports:
> http://crash-stats.mozilla.com/report/list?product=Fennec&query_search=signature&range_value=4&range_unit=weeks&signature=mozilla%3A%3Adom%3A%3AContentParent%3A%3AObserve
> There are no crashes with this signature from 4.0b3pre/20101126 build, that is
> 4 consecutive builds without crashes.

As far as I can tell, that could only mean that the existing code which people hit and caused them to crash has been mitigated, but the code which has caused the crash to happen in the first place still lives in the tree.  Unless we don't have an analysis showing why the pref service pointer can *never* be null there, this but is not fixed.

Comment 7

[Doug Turner (:dougt)]

ehsan, i do not understand your analysis.  We checked in changes to android startup sequence which were incorrect.  During this time when the bad changes were in the tree crashes started happening -- especially if you rotated the screen.  This cause this specific bug and of course bug 614801.  We backed out that change on the night of the 25th.  On the 26th, the problem was gone.

There are all sorts of bugs that happen when the startup sequence of gecko is wrong.  Do we worry about those?  Of course not.  How is this bug any different?

Comment 8

[(no longer active)]

(In reply to comment #7)
> ehsan, i do not understand your analysis.  We checked in changes to android
> startup sequence which were incorrect.  During this time when the bad changes
> were in the tree crashes started happening -- especially if you rotated the
> screen.  This cause this specific bug and of course bug 614801.  We backed out
> that change on the night of the 25th.  On the 26th, the problem was gone.
> 
> There are all sorts of bugs that happen when the startup sequence of gecko is
> wrong.  Do we worry about those?  Of course not.  How is this bug any
> different?

Is this something which could be possibly affected by add-ons?  I have a bunch installed, and I disable/enable them very frequently, so I don't remember which ones were enabled when I hit this crash.  But is this possible to be triggered by an add-on?

If not, is this possible to get triggered by another change in the future, which is not necessarily an incorrect change in the startup sequence?

Comment 9

[Doug Turner (:dougt)]

> Is this something which could be possibly affected by add-ons?

sure addons could mess with startup ordering, but this didn't happen in this case.

Comment 10

[(no longer active)]

(In reply to comment #9)
> > Is this something which could be possibly affected by add-ons?
> 
> sure addons could mess with startup ordering, but this didn't happen in this
> case.

Well, is that enough reason to not fix this, by just assuming that add-ons will never do this?

Comment 11

[Doug Turner (:dougt)]

yes.

Comment 12

[(no longer active)]

OK, then.

For the record, I don't agree with your decision, but it's ultimately your call.

Comment 13

[Doug Turner (:dougt)]

actually, i am wrong... I do not think an addon could change the java code that cause this regression.

Comment 14

[(no longer active)]

(In reply to comment #13)
> actually, i am wrong... I do not think an addon could change the java code that
> cause this regression.

Sorry for reiterating over this, but is that (altered startup sequence) the only case where the pref service pointer can be null there?

Comment 15

[Doug Turner (:dougt)]

good question.  a null check might be warranted.  however, i think i'd rather crash and fix it..  what do you think?

Comment 16

[(no longer active)]

(In reply to comment #15)
> good question.  a null check might be warranted.  however, i think i'd rather
> crash and fix it..  what do you think?

Hmm, what's the downside of just taking the patch posted for the null-check?

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

I'd prefer to have the null check. The argument for crashing is that issues are more debug-able, but if you use an NS_ENSURE_TRUE you get the same debug-ability.

Comment 18

[Josh Matthews [:jdm]]

(In reply to comment #16)
> (In reply to comment #15)
> > good question.  a null check might be warranted.  however, i think i'd rather
> > crash and fix it..  what do you think?
> 
> Hmm, what's the downside of just taking the patch posted for the null-check?

I'm loathe to do so.  It doesn't seem like we should be in a position where preferences are being updated but we don't have the service available.

Comment 19

[(no longer active)]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > good question.  a null check might be warranted.  however, i think i'd rather
> > > crash and fix it..  what do you think?
> > 
> > Hmm, what's the downside of just taking the patch posted for the null-check?
> 
> I'm loathe to do so.  It doesn't seem like we should be in a position where
> preferences are being updated but we don't have the service available.

What's guaranteeing that we're never in that position?  Unless we have such an analysis available, the point you're raising doesn't seem valid to me.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #18)
> I'm loathe to do so.  It doesn't seem like we should be in a position where
> preferences are being updated but we don't have the service available.
We shouldn't just crash though. If need be we should explicitly abort.

Comment 21

[[e-mail]]

(In reply to comment #20)
> (In reply to comment #18)
> > I'm loathe to do so.  It doesn't seem like we should be in a position where
> > preferences are being updated but we don't have the service available.
> We shouldn't just crash though. If need be we should explicitly abort.

I agree with jdm. If this happens we don't want to handle it, we want to explode so we can fix the bug. I'd be fine with an abort, but I don't think it's a huge deal.