Closed Bug 1450266 Opened 3 years ago Closed 3 years ago

Remove nsGlobalWindowInner::CleanUp in favor of FreeInnerObjects()

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(7 files, 6 obsolete files)

14.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.74 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.29 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.58 KB, patch
smaug
: review+
Details | Diff | Splinter Review
948 bytes, patch
bkelly
: review+
Details | Diff | Splinter Review
1.48 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
1.48 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
In bug 1447871 I realized that we have two different cleanup methods in nsGlobalWindowInner.  CleanUp() and FreeInnerObjects().  I would like to unify them into just FreeInnerObjects() for a number of reasons:

1. There are comments suggesting that CleanUp was originally intended for outer windows and was not intended for inner windows:

https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/dom/base/nsGlobalWindowInner.cpp#1127-1130

2. In addition, we called CleanUp() and FreeInnerObjects() inconsistently.

CleanUp() is called from:
 - ~nsGlobalWindowInner()
 - nsGlobalWindowOuter::CleanUp() which in turn gets called from DetachFromDocshell() and ReallyCloseWindow().

FreeInnerObjects() is called from:
 - ~WindowStateHolder()
 - nsGlobalWindowOuter::SetNewDocument()
 - nsGlobalWindowOuter::DetachFromDocshell()

So the only case where we were consistently executing all cleanup code was in DetachFromDocshell().  Otherwise we mostly only ran FreeInnerObjects() code and then waited for cycle collection to delete the inner window so we could run CleanUp() from ~nsGlobalWindowInner().

3. DOMEventTargetHelper classes often need to keep themselves alive if they have listeners until the window is closed.  DETH uses DisconnectFromOwner() to detect this condition:

https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/dom/events/DOMEventTargetHelper.cpp#154

However, DisconnectFromOwner() is only triggered from the window's CleanUp() for right now.  Since we don't call that consistently its possible for DETH to cause a leak unless the extending class does a work around like BroadcastChannel:

https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/dom/broadcastchannel/BroadcastChannel.cpp#494-519

MediaQueryList and ServiceWorker binding objects also have similar issues.

I'm going to move some patches for fixing this from bug 1447871 over to here to land on nightly.  That bug needs to land with a short term fix first so it can be uplifted.
Blocks: 1450269
Blocks: 1450271
Summary: Remove nsGLobalWindowInner::CleanUp in favor of FreeInnerObjects() → Remove nsGlobalWindowInner::CleanUp in favor of FreeInnerObjects()
Rebase.  Also make sure we continue to call cleanup code from the destructor, etc.
Attachment #8963958 - Attachment is obsolete: true
Re-numbered this patch.
Attachment #8963963 - Attachment is obsolete: true
Comment on attachment 8964030 [details] [diff] [review]
P1 Remove nsGlobalWindowInner::CleanUp() method in favor of FreeInnerObjects(). r=smaug

Olli, this patch folds nsGlobalWindowInner::CleanUp() into FreeInnerObjects().  I've written about why I think we should do this in a few places (like comment 0 and bug 1447871), but I will briefly summarize here:

1. Two separate cleanup operations are confusing to developers.  Many people don't know where they should put their cleanup code so they end up putting it in both places, etc.
2. CleanUp() and FreeInnerObjects() are called inconsistently.  Seem comment 0 from the call sites.  The main effect of the difference, though, is that when an outer window is destroyed its current inner window gets CleanUp() called immediately.  For inners in bfcache or being replaced via document.open() CleanUp() is only called if the window is de-refed and the destructor is called.
3. Because of the inconsistency in (2) we have code that might think its getting consistently cleaned up, but is not.  For example, DETH::KeepAliveIfEventListenersFor() adds a self reference that is cleared in DisconnectFromOwners().  Because of (2), however, DisconnectFromOwners() may not be called until after this self-ref is released.  This causes memory leaks.  The service worker leak was similar to this.

I think it would make things much easier to maintain if we had a single cleanup method on nsGlobalWindowInner().  My proposal here is to make that FreeInnerObjects().

Note, there is some duplicate code between CleanUp() and FreeInnerObjects().  In this patch I just did a simple move of CleanUp().  The P2 patch will de-dupe the overlap.

This patch also gets rid of the mCleanedUp member and its associated getters.  Instead I propose that we just use nsIGlobalObject::IsDying().  I will further remove the InnerObjectsFreed() method in favor of IsDying() in P3.

Finally, I added a call to FreeInnerObjects() to ~nsGlobalWindowInner() as a replacement for the CleanUp() method there.  In theory FreeInnerObjects() should always be called before destruction, but there could be some error paths where we go straight to the destructor early.  I use IsDying() to avoid double-execution of FreeInnerObjects().
Attachment #8964030 - Flags: review?(bugs)
Comment on attachment 8964031 [details] [diff] [review]
P2 Remove duplicate cleanup code from FreeInnerObjects(). r=smaug

This removes code from FreeInnerObjects() that is now duplicated after moving CleanUp() over in P1.
Attachment #8964031 - Flags: review?(bugs)
Comment on attachment 8964032 [details] [diff] [review]
P3 Remove nsGlobalWindowInner::InnerObjectsFreed() in favor of IsDying(). r=smaug

This patch removed mInnerObjectsFreed and its associate getter.  In its place nsIGlobalObject::IsDying() is used instead.

This helps simplify things by having a single getter code can check instead of having to figure out which one to pick.  It also moves us closer to a world where code does not need to know if it has a window or worker scope because it can just call things on nsIGlobalObject.
Attachment #8964032 - Flags: review?(bugs)
Comment on attachment 8964033 [details] [diff] [review]
P4 Rebind DETH objects to the new global when document.open() is called. r=smaug

This makes document.open() rebind the list of DETH objects from the old window onto the new window.  This seems somewhat dubious to me, but its the only sane way I can see to keep test_bug372964-2.html passing.  I filed bug 1449992 to consider achieving this behavior by simply reusing the window instead of replacing it.
Attachment #8964033 - Flags: review?(bugs)
No longer blocks: 1450271
Blocks: 1450358
Attachment #8964032 - Flags: review?(bugs) → review+
Comment on attachment 8964033 [details] [diff] [review]
P4 Rebind DETH objects to the new global when document.open() is called. r=smaug

This shouldn't be less correct than the current setup.
Attachment #8964033 - Flags: review?(bugs) → review+
Attachment #8964031 - Flags: review?(bugs) → review+
Attachment #8964030 - Flags: review?(bugs) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d096fbbe39
P1 Remove nsGlobalWindowInner::CleanUp() method in favor of FreeInnerObjects(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/96116f87449e
P2 Remove duplicate cleanup code from FreeInnerObjects(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5238cc7816
P3 Remove nsGlobalWindowInner::InnerObjectsFreed() in favor of IsDying(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda31f55d8ca
P4 Rebind DETH objects to the new global when document.open() is called. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4fc9d3b1f6
P5 Make MIDIInput::Receive() check for nullptr GetOwner(). r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/a880dc2774c1
P6 Make IDBDatabase invalidate any transactions on DisconnectFromOwner(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc66bed8784f
P7 Make XMLHttpRequestMainThread check for a valid inner window before dispatching events. r=baku r=smaug
Duplicate of this bug: 1450713
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.