784792 - "ASSERTION: win should be closed or closing" with window.open, gcslice

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Enable popups:
      user_pref("dom.disable_open_during_load", false);
3. Load the testcase.

Result #1 is an assertion deep within nsIDOMEvent_GetTarget:

###!!! ASSERTION: win should be closed or closing: 'win->IsClosedOrClosing()', file dom/base/nsDOMClassInfo.cpp, line 2080

Result #2 is a JS exception, thrown into pageShowEventHandlers:

JavaScript error: chrome://browser/content/browser.js, line 6378: NS_ERROR_FAILURE: Failure

These are the symptoms of bug 762753 and bug 742139, respectively.  But this testcase does not involve XSLT!  This bug might hold clues to the oranges hidden in bug 782167.

Comment 1

[Jesse Ruderman]

Attachment: stack trace & js error

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jesse Ruderman from comment #0)
> Result #2 is a JS exception, thrown into pageShowEventHandlers:
> 
> JavaScript error: chrome://browser/content/browser.js, line 6378:
> NS_ERROR_FAILURE: Failure

This sounds like bug 780594, which has become a top-orange relatively recently.

Comment 3

[Andrew McCreight [:mccr8]]

Bill and I tried looking at this, but we couldn't make sense of it. The code just seems to be doing what it is supposed to be doing: you try to wrap a window that is dying and has a null global, and you produce an error, or something like that.  Maybe Bobby, who added this check in there, can produce a more useful analysis.

Comment 4

[Andrew McCreight [:mccr8]]

Bobby, can you take a look at this?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Bobby, can you take a look at this?

It's on my list.

Comment 6

[Bobby Holley (:bholley)]

Hm, so does indeed appear like nothing super abnormal is happening. The window gets opened, a ref to the inner window is stashed in an event, and then the inner window is destroyed and replaced in a call to nsGlobalWindow::SetNewDocument. The important bit here is that the window is torn down (with FreeInnerObjects) without setting any of the state that factors into IsClosedOrClosing().

I was going to ask somebody about the origin of the assertion, but then it turns out that _I_ added it (!!) in bug 691178. So it looks like I just mistakenly assumed that any torn-down window would be considered "closed" (when the truth is more complicated).

The original point of this assertion was to make sure that any null window JS object during PreCreate was the result of a window having been already torn down, as opposed to the window not having been created yet. But I don't think that's a super relevant concern now that I rewrote all the bootstrapping code and landed CPG; the window's JS object is the global, and if it didn't exist we wouldn't even have a compartment to be working in.

So we could add more state (mTornDown) to nsGlobalWindow. But I think this assertion is just more of a pain point for everyone than it's worth. So let's remove it.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Remove IsClosedOrClosing() assertion. v1

We don't assert with this patch (obviously), but we still throw here when creating a wrapper for the torn-down window:

http://hg.mozilla.org/mozilla-central/file/7585c9b91877/browser/base/content/browser.js#l223

Is that ok? I don't see an obvious alternative, but maybe I don't have the full picture.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 659920 [details] [diff] [review]
Remove IsClosedOrClosing() assertion. v1

Requesting feedback from gavin about comment 7.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 659920 [details] [diff] [review]
Remove IsClosedOrClosing() assertion. v1

From my naive point of view, this pagehide event shouldn't be fired if its target is an element that's torn down enough that just trying to access it throws - either we should be firing the event before the tear down happens, or we shouldn't be firing the event.

Do you know why this started happening relatively recently?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Do we fire pagehide after teardown?  I'd expect it before. Note that the testcase uses an interval timer, not a pagehide; the stack to the assert is certainly through the interval timer....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 659920 [details] [diff] [review]
Remove IsClosedOrClosing() assertion. v1

r=me on this part

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Oh, yeah, doing the target check in the delayed function is kind of dumb. We should just fix that, per bug 786940 comment 3.

Comment 13

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66eedfc80913

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Do we fire pagehide after teardown?  I'd expect it before. Note that the
> testcase uses an interval timer, not a pagehide; the stack to the assert is
> certainly through the interval timer....

I think the issue with the testcase has to do with the fact that it's a data URI, so it loads sync, and therefore evicts the old inner window sync.

Philor says that the fact that we throw here is causing lots of oranges. Is there anything we can do? If we throw, does that indicate a browser-chrome bug, or could the DOM be smarter?

Comment 15

[Boris Zbarsky [:bzbarsky]]

data: URIs load async like every other URI (modulo the font-face rule hackery).

For the rest, ccing Olli.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66eedfc80913

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> Oh, yeah, doing the target check in the delayed function is kind of dumb. We
> should just fix that, per bug 786940 comment 3.

I fixed this in bug 780594.

Comment 18

[Alex Keybl [:akeybl]]

Bug 780594 implies that only FF17/18 are affected. Just want to be clear - is the ESR affected as well?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #18)
> Bug 780594 implies that only FF17/18 are affected. Just want to be clear -
> is the ESR affected as well?

Well, this bug just removed an assertion, and didn't turn out to be security-sensitive. I don't think it needs to be tracked.

Opening this bug up.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e601c1693713

Comment 21

[Dão Gottwald [:dao]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #20)
> https://hg.mozilla.org/releases/mozilla-beta/rev/e601c1693713

Why did this land without approval?

Comment 22

[Ed Morley [:emorley]]

(In reply to Dão Gottwald [:dao] from comment #21)
> (In reply to Ed Morley [:edmorley UTC+1] from comment #20)
> > https://hg.mozilla.org/releases/mozilla-beta/rev/e601c1693713
> 
> Why did this land without approval?

I hg transplanted a dozen or so test-only orange fixes by cross referencing with OrangeFactor - I had vetted all the diffs to ensure test-only, but two non-test had slipped through my bad.

Backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/903136287770

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 659920 [details] [diff] [review]
Remove IsClosedOrClosing() assertion. v1

bholley, is this reasonable to land on beta?  I can deal with all of the request/landing foo if so.  It seems like it might be worth it just to avoid ESR-17 random orange for a bogus assertion...

Comment 24

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #23)
> Comment on attachment 659920 [details] [diff] [review]
> Remove IsClosedOrClosing() assertion. v1
> 
> bholley, is this reasonable to land on beta?  I can deal with all of the
> request/landing foo if so.  It seems like it might be worth it just to avoid
> ESR-17 random orange for a bogus assertion...

Yes, this is fine to land on beta.

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 659920 [details] [diff] [review]
Remove IsClosedOrClosing() assertion. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): something in 17
User impact if declined: none
Testing completed (on m-c, etc.): it has been on 18 and 19 for a while
Risk to taking this patch (and alternatives if risky): very low, it only removes a bogus assertion that causes random oranges on TBPL.
String or UUID changes made by this patch: none

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 659920 [details] [diff] [review]
Remove IsClosedOrClosing() assertion. v1

glad this might prevent some ESR-17 rando-orange, approving for uplift.

Comment 27

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b764d990ccad

Comment 28

[Manuela Muntean [Away]]

I didn't get the errors provided in the description, using the latest beta debug build : ozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0,   Build ID: 20121022223813

Comment 29

[Manuela Muntean [Away]]

This issue isn't reproducible anymore using the STR from the description and the latest beta debug build.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20121122190812