646184 - Crash [@ nsGlobalWindow::GetLocalStorage] getting localStorage from removed frame

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

First seen 2011-03-28 1:40pm.  Might be a regression from the cedar merge :(

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

Oh I bet it's a regression from bug 639849.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Possible if someone nulls out only one of mDoc and mDocument.  Ms2ger, can you look?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  They're probably just both null and the code lost the null-check.

Jesse, are you patching?  If not, please reassign to Ms2ger?

Comment 5

[Jesse Ruderman]

I must have accidentally clicked "Take" and not noticed that it made a change (rather than simply making a new field visible).

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1: reinstate null checks.

In that case, this should fix it. Jesse, could you test?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522812 [details] [diff] [review]
Patch v1: reinstate null checks.

r=me

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin.

Comment 9

[(no longer active)]

Boris landed this on cedar, but I had to back it out because of oranges like this:

http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301693776.1301694328.13610.gz

Original landing: http://hg.mozilla.org/projects/cedar/rev/649f50ed53ca
Backout: http://hg.mozilla.org/projects/cedar/rev/779e2ba9810b

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Annotated assertion.

Unless someone has a better idea, I'll just have to annotate the test.

Comment 11

[(no longer active)]

(In reply to comment #10)
> Created attachment 523768 [details] [diff] [review]
> Annotated assertion.
> 
> Unless someone has a better idea, I'll just have to annotate the test.

Seems like a bad idea to me.  We should understand the assertion.  CCing some people who might know stuff about xpconnect.

Comment 12

[Jesse Ruderman]

The orange is the new test hitting the assertion

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!  (stack and details follow): 'Error', file /builds/slave/ced-osx64-dbg/build/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 761

dbaron can probably tell us which existing bug report covers it ;)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think we should check this in ... the XPConnect assertion happens quite often on many different things including session restore.

Comment 14

[(no longer active)]

(In reply to comment #13)
> I think we should check this in ... the XPConnect assertion happens quite often
> on many different things including session restore.

I would be fine with us landing this with a assertion annotations _if_ we knew that the triggered assertion is an instance of an existing bug.  So far, I've not seen anything which suggests this is true.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ehsan and I chatted on IRC a bit about this.  Does the assertion happen before and after the fix with the testcase?  If the assertion is added by the fix, then we need to figure it out.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: details from log in comment 9

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think the assertion:
 * is not caused by the patch
 * might or might not have the same underlying cause as bug 631289
 * is not related to any of the existing no-Components assertion bugs

Comment 18

[(no longer active)]

(In reply to comment #17)
> I think the assertion:
>  * is not caused by the patch
>  * might or might not have the same underlying cause as bug 631289
>  * is not related to any of the existing no-Components assertion bugs

In that case, I'd be fine with this patch landing if you file a new bug for the assertion, and add the bug # to the reftest manifest annotating the assertion as a comment.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/0f2d31305fca

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Looks like the assertion went away with today's tracemonkey merge (the current unexpected-pass orange).

Comment 21

[Martijn Wargers (dead)]

Still crashes in Fennec5.0 beta.

Comment 24

[David Maciejak]

just stumble upon today with my fuzzer using latest ubuntu linux and firefox 5.0,
here an extract from gdb

Program received signal SIGSEGV, Segmentation fault.
0xb7115955 in nsGlobalWindow::GetLocalStorage (this=0xa19ec0b0, aLocalStorage=0xbfffde4c)
    at /build/buildd/firefox-5.0+build1+nobinonly/build-tree/mozilla/dom/base/nsGlobalWindow.cpp:7987


regards,
david

Comment 25

[Martijn Wargers (dead)]

Which fuzzer?

Comment 26

[David Maciejak]

a private one ;)