Closed Bug 932309 Opened 11 years ago Closed 11 years ago

Don't null out mDoc in nsGlobalWindow::FreeInnerObjects

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(4 files)

Attached patch v1 (deleted) — Splinter Review
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.
Attachment #824019 - Flags: review?(bugs)
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.
Blocks: 932322
Attachment #824019 - Flags: review?(bugs) → review+
Attached patch Additional fixes v1 (deleted) — Splinter Review
Attachment #825986 - Flags: review?(bugs)
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;
}
Attachment #825986 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e30f630191b9
https://hg.mozilla.org/mozilla-central/rev/d4fdcf5f3c04
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attached patch better debug message (deleted) — Splinter Review
Attachment #828313 - Flags: review?(anygregor)
Depends on: 935786
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.
Attachment #828313 - Flags: review?(anygregor) → review+
I need to null check event.
Attached patch +null check (deleted) — Splinter Review
The method throws if null is passed.
Depends on: 937318
Blocks: 951245
Peter, how reasonable is it to backport this to beta 27?  The fix for bug 936056 basically depends on this patch...
Flags: needinfo?(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.
Flags: needinfo?(peterv)
Target Milestone: mozilla28 → ---
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: