932309 - Don't null out mDoc in nsGlobalWindow::FreeInnerObjects

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

If we stop using a permanent window.document property by instead relying on WebIDL for that we need to stop nulling out mDoc in nsGlobalWindow::FreeInnerObjects because otherwise window.document becomes null. Turns out we rely on mDoc being null to avoid firing certain events in nsGlobalWindow. I've fixed that by doing a check for "is current inner window" instead. I though about doing the check in a more central place like DispatchEvent, but then the callers would still be doing a bunch of work (creating events etc) for all non-current inner windows, just to throw it all away again. So I've opted to assert in DispatchEvent instead. Let me know how you feel about that.

Comment 1

[Peter Van der Beken [:peterv]]

The GamepadServiceTest fix is just to avoid a crash when running some of the gamepad tests on their own, which happened while I was debugging this patch.

Comment 2

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6878c4ee086

Comment 3

[Ed Morley [:emorley]]

Backed out for assertions, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29961392&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/2137bf09cdc1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Additional fixes v1

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 825986 [details] [diff] [review]
Additional fixes v1


> #ifdef MOZ_B2G
>   if (!nsCRT::strcmp(aTopic, NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC) ||
>       !nsCRT::strcmp(aTopic, NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC)) {
>+    if (!IsInnerWindow() || !IsCurrentInnerWindow()) {
>+      return NS_OK;
>+    }

MOZ_ASSERT(IsInnerWindow());
if (!IsCurrentInnerWindow()) {
  return NS_OK;
}

Comment 6

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e3bcb78784e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b99a0f6ff1b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for making Linux32 opt mochitest-bc perma-orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d4847dfd5c

https://tbpl.mozilla.org/php/getParsedLog.php?id=30087228&tree=Mozilla-Inbound

Comment 8

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e30f630191b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fdcf5f3c04

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e30f630191b9
https://hg.mozilla.org/mozilla-central/rev/d4fdcf5f3c04

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better debug message

Comment 11

[Gregor Wagner [:gwagner]]

Comment on attachment 828313 [details] [diff] [review]
better debug message

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

I don't know why, but this gives us a stack as well.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I need to null check event.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +null check

The method throws if null is passed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Peter, how reasonable is it to backport this to beta 27?  The fix for bug 936056 basically depends on this patch...

Comment 15

[Peter Van der Beken [:peterv]]

Bz is going to try to backport bug 936056 to branch without this fix, because it's a bit scary to backport this one.