553107 - browser_354894.js triggering "XPConnect is being called on a scope without a 'Components' property!"

Status: RESOLVED WORKSFORME

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Tests still pass, but there's a long pause as this is triggered. I'm thinking this might have been causing bug 528219.

I haven't had a chance to test all OSes, just OS X, but it's likely happening on all platforms.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Tracked this down to http://hg.mozilla.org/mozilla-central/file/d7ba6aef21eb/browser/components/sessionstore/test/browser/browser_354894.js#l502

It's happening because the window is already destroyed, so xpconnect thinks we're doing something wrong. So we assert and boo, that sucks. We can work around it though by checking for something instead of window.closed.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Check that we receive an additional "browser-lastwindow-close-granted" notification instead of checking window.closed.

Also made the mac test simpler instead of needlessly opening 5 extra windows and removed the timeout increase.

Comment 3

[Simon Bünzli]

Does that mean that window.closed is essentially pointless, because instead of (or additionally to) returning |true| it asserts? If so, why not fix window.closed instead of working around it in our test?

BTW: /Mac/.test(platform) is the preferred alternative to platform.match(/Mac/) when you're not interested in the array of matched groups.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> Does that mean that window.closed is essentially pointless, because instead of
> (or additionally to) returning |true| it asserts? If so, why not fix
> window.closed instead of working around it in our test?

Sooo here's what Blake explained to me...
"the XPConnect bits would still be around, but all of your "expando" properties or global variables would go away. We have a bug on this, but the general problem is that, to avoid leaks, we clear away the entire window object. But XPConnect resolves its stuff lazily anyway. so asking for something like window.closed works because we'll re-get the property. the problem is that a lot of times, running code in a closed window  does sometimes mean that someone made a mistake."

Additionally, it's only an assertion in debug builds. DEBUG_CheckForComponentsInScope is a noop otherwise.

Blake said I could file a bug to warn instead. I actually think it's worth asserting for the cases of legitimate problems.

> BTW: /Mac/.test(platform) is the preferred alternative to platform.match(/Mac/)
> when you're not interested in the array of matched groups.

Ah true. I just copy+pasted from farther down in the file.

Comment 5

[Simon Bünzli]

Shouldn't we be getting be getting assertions in all of the following cases?

http://mxr.mozilla.org/mozilla-central/search?string=(win(dow)*|getNext\(\))\.closed&regexp=1

Since checking whether a window has been closed seems to be that common, I'd really not have to start using work-arounds. If you want to keep the assertion and it's not possible to make an exception for window.closed, then I'd rather have a helper method on nsIDOMWindowUtils for checking whether a window's still alive and then use that instead.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Not sure if this is even an issue anymore with CPG...

Comment 7

[:Cykesiopka]

Bug 747434 removed the assertion.