Closed Bug 1415645 Opened 2 years ago Closed 2 years ago

Switch nsGlobalWindow internals to use {Inner,Outer} variants explicitly where possible

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

Attachments

(5 files, 3 obsolete files)

Follow up from bug 1414974 to change the internals in preparation for a full split.
This will make the split easier, as this method is used by both
nsGlobalWindowInner and nsGlobalWindowOuter.

MozReview-Commit-ID: FsjK4y6x7NE
Attachment #8926526 - Flags: review?(bugs)
After the window split is complete, the inner window linked list won't be
homogenously typed anymore, as there will be an nsGlobalWindowOuter member in
addition to the nsGlobalWindowInner members. This patch changes the code to
perform void* pointer comparisons before casting to nsGlobalWindowInner to avoid
this issue.

MozReview-Commit-ID: 56q5XodtGe7
Attachment #8926527 - Flags: review?(bugs)
MozReview-Commit-ID: ANdBkuhZ2xx
Attachment #8926528 - Flags: review?(bugs)
Comment on attachment 8926524 [details] [diff] [review]
Part 1: Make nsGlobalWindow.cpp use the specific {Inner,Outer} variants in more places internally

@@ -12559,17 +12561,19 @@ nsGlobalWindow::FreezeInternal()
   if (mFreezeDepth != 1) {
     return;
   }
 
   mozilla::dom::workers::FreezeWorkersForWindow(AsInner());
 
   mTimeoutManager->Freeze();
 
-  NotifyDOMWindowFrozen(this);
+  if (IsInnerWindow()) {
+    NotifyDOMWindowFrozen(AssertInner());
+  }
 }

We do assert in that method that we're dealing with inner.
And same with nsGlobalWindow::ThawInternal()

FromSupports methods could use static_cast, no?
Attachment #8926524 - Flags: review?(bugs) → review+
Comment on attachment 8926526 [details] [diff] [review]
Part 2: Make CleanupCachedXBLHandlers a instance method


> 
> // static
> void
>-nsGlobalWindow::CleanupCachedXBLHandlers(nsGlobalWindow* aWindow)
>+nsGlobalWindow::CleanupCachedXBLHandlers()
Well, the comment above still say this is static. Please remove that
Attachment #8926526 - Flags: review?(bugs) → review+
Attachment #8926527 - Flags: review?(bugs) → review+
Attachment #8926528 - Flags: review?(bugs) → review+
After the window split is complete, the inner window linked list won't be
homogenously typed anymore, as there will be an nsGlobalWindowOuter member in
addition to the nsGlobalWindowInner members. This patch changes the code to
perform void* pointer comparisons before casting to nsGlobalWindowInner to avoid
this issue.

MozReview-Commit-ID: 56q5XodtGe7
void* is hard -- This patch fixes the code to correctly use PRCList* instead
which means that we don't mess up offsets and read random memory.

MozReview-Commit-ID: 56q5XodtGe7
Attachment #8926527 - Attachment is obsolete: true
Attachment #8926900 - Attachment is obsolete: true
Attachment #8926901 - Attachment is obsolete: true
Attachment #8926981 - Flags: review?(bugs) → review+
I ran into these when I started to write the split patch.

There's some code here which we never seem to call which is broken - perhaps we want to remove it in the future?

MozReview-Commit-ID: DvMH4cVw5NK
Attachment #8927004 - Flags: review?(bugs)
Comment on attachment 8927004 [details] [diff] [review]
Part 5: Fix some more consumers of nsGlobalWindow which I missed


>+++ b/dom/base/test/gtest/TestParserDialogOptions.cpp
>@@ -2,16 +2,18 @@
> /* vim: set ts=8 sts=2 et sw=2 tw=80: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "gtest/gtest.h"
> #include "nsGlobalWindow.h"
> 
>+// XXX(nika): I can't find any non-test callers of this code, can we remove it?
>+
ask baku, but it isn't even clear what your question refers to.

Not sure about those other XXXs. I guess they are fine as such
Attachment #8927004 - Flags: review?(bugs) → review+
baku. The file TestParserDialogOptions.cpp calls nsGlobalWindow::TokenizeDialogOptions and nsGlobalWindow::ConvertDialogOptions, both of which are functions which appear to not be called anywhere else in the codebase.

Can we remove the test and those functions?
Flags: needinfo?(amarchesini)
> Can we remove the test and those functions?

Sure.
Flags: needinfo?(amarchesini)
Blocks: 1416384
Priority: -- → P2
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdfee217a97
Part 1: Make nsGlobalWindow.cpp use the specific {Inner,Outer} variants in more places internally, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6c2c5490f4
Part 2: Make CleanupCachedXBLHandlers a instance method, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a766cc68671
Part 3: Don't cast to nsGlobalWindow when working with the inner window linked list, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c0344711b6
Part 4: Modify some shared runnables to not refer directly to nsGlobalWindow, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56352d90f67
Part 5: Fix some more consumers of nsGlobalWindow which I missed, r=smaug
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.