Markus Stange [:mstange] Reporter
	Description • 7 years ago

If you close a window with a lot of tabs, or a tab with a lot of iframes, we dispatch lots of WindowDestroyedEvents at the same time. I'm worried that these events can clog up the event queue and stop us from processing other events that are more important for responsiveness.

I know that I've seen this problem in profiles for years, but I haven't been able to reproduce it just now, by closing a window with 80 searchfox.org tabs. Maybe that's because searchfox doesn't contain iframes or plugins. Or maybe other scheduling changes have solved this problem in the meantime?

Assuming the problem still exists, could we use NS_IdleDispatchToCurrentThread for these events instead?
Since these events also free up memory, we should probably have a way to prevent these events from being deferred indefinitely. For example, we could react to memory pressure events by just flushing all pending WindowDestroyedEvents.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 7 years ago

Yeah, those would need to use idle dispatch with timeout, to ensure some relatively fast handling.
(Not sure we get too useful memory pressures.)
But better to capture at least some profile showing this is actually an issue these days.

	Andrew McCreight [:mccr8]
	Comment 2 • 7 years ago

The main slowdowns I'm aware of for WindowDestroyedEvents is compartment nuking, and bug 816784 should improve that.

	(no longer active) Assignee
	Comment 3 • 7 years ago

(In reply to Andrew McCreight [:mccr8] from comment #2)
> The main slowdowns I'm aware of for WindowDestroyedEvents is compartment
> nuking, and bug 816784 should improve that.

I'm wondering if it will make it so that it will be guaranteed to be *much* less than a frame budget at all times after that bug.  That seems...  unlikely?  :-/

(In reply to Olli Pettay [:smaug] from comment #1)
> But better to capture at least some profile showing this is actually an
> issue these days.

Going back in memory of the bugs I have files I managed to find this profile from reloading Facebook with 50ms of WindowDestroyedEvents in two separate chunks of timeline: https://perfht.ml/2oVXMqN.  I've seen it pretty often anecdotally.  I think we should think about mitigating the case where the user input events keep coming in, and then just move this over to idle dispatch, *and* fix bug 816784 at the same time.

	Pulsebot
	Comment 5 • 7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac329aeaaf9
Dispatch WindowDestroyedEvent to the idle queue; r=smaug

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 7 years ago

Backed out for various dead window related test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e761a94c2dc7e5fddb9ce5b160468ac6763daf69

https://treeherder.mozilla.org/logviewer.html#?job_id=101745289&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=101745221&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=101745304&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=101745347&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=101745281&repo=mozilla-inbound

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 7 years ago

https://hg.mozilla.org/projects/cedar/rev/4ac329aeaaf94f6a9316a3f562e465db2dd24675
Bug 1361461 - Dispatch WindowDestroyedEvent to the idle queue; r=smaug

	(no longer active) Assignee
	Comment 8 • 7 years ago

Comment on attachment 8870987 [details] [diff] [review]
Dispatch WindowDestroyedEvent to the idle queue

This approach proved to be impractical due to having to fix thousands of failing tests all over the tree.  The reason why those tests were failing was the changing of the timing of the inner/outer-window-destroyed events which a lot of things depend on apparently.

I ended up employing a slightly different approach.  The inner/outer-window-destroyed events is actually not the interesting part of the runnable for our purposes here since the compartment nuking is the time consuming part.  So I decided to break this task off into two phases, adding a second nuking phase which will run off of idle dispatch and will send the inner/outer-window-nuked events that can be used in the few tests that actually care about compartment nuking rather than window closing.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 7 years ago

Comment on attachment 8875064 [details] [diff] [review]
Dispatch the compartment-nuking part of WindowDestroyedEvent to the idle queue


>-    nsCOMPtr<nsISupports> window = do_QueryReferent(mWindow);
>-    if (!skipNukeCrossCompartment && window) {
>-      nsGlobalWindow* win = nsGlobalWindow::FromSupports(window);
>-      nsGlobalWindow* currentInner = win->IsInnerWindow() ? win : win->GetCurrentInnerWindowInternal();
>-      NS_ENSURE_TRUE(currentInner, NS_OK);
>+        if (!skipNukeCrossCompartment) {
>+          // The compartment nuking phase might be too expensive, so do that
>+          // part off of idle dispatch.
> 
>-      AutoSafeJSContext cx;
>-      JS::Rooted<JSObject*> obj(cx, currentInner->FastGetGlobalJSObject());
>-      if (obj && !js::IsSystemCompartment(js::GetObjectCompartment(obj))) {
>-        JSCompartment* cpt = js::GetObjectCompartment(obj);
>-        nsCOMPtr<nsIPrincipal> pc = nsJSPrincipals::get(JS_GetCompartmentPrincipals(cpt));
>+          // For the compartment nuking phase, we dispatch either an
>+          // inner-window-nuked or an outer-window-nuked notification.
>+          // This will allow tests to wait for compartment nuking to happen.
>+          mTopic.ReplaceSubstring("destroyed", "nuked");
This is a bit ugly. Would be nicer to just assign the right value, then the assertion wouldn't be needed.
but up to you


> private:
>   uint64_t mID;
>+  uint32_t mPhase;
Please, some enum for this.

	Pulsebot
	Comment 11 • 7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/962439237d24
Dispatch the compartment-nuking part of WindowDestroyedEvent to the idle queue; r=smaug

	Pulsebot
	Comment 12 • 7 years ago

Backout by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6353d0ebd86f
Backout due to potentially introducing some intermittent reftest failures

	(no longer active) Assignee
	Comment 13 • 7 years ago

The reftest failures were either showing up as shutdown timeouts or leaks, which seemed to indicate that we had ended up with a whole bunch of nuking tasks which were never run until the end of the test suite.  As a fix I pushed a change which added a timeout of 1 second to the idle dispatch to make sure the cleanup eventually runs, and that seems to have worked well:

https://treeherder.mozilla.org/#/jobs?repo=try&author=eakhgari@mozilla.com

I retriggered R1 on Linux32 debug, Ru2 on Windows 7 debug, and R and Ru on Windows 7 opt (that last one looks bad but those are unrelated intermittents.)

I'm going to reland this now.

	Pulsebot
	Comment 14 • 7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/575e351a12af
Dispatch the compartment-nuking part of WindowDestroyedEvent to the idle queue; r=smaug

	Carsten Book [:Tomcat]
	Comment 15 • 7 years ago

sorry had to back this out for frequent r5 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105425298&repo=mozilla-inbound

	Pulsebot
	Comment 16 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82bdfe54a6cd
Backed out changeset 575e351a12af for causing frequent reftest assertion failures like Assertion failure: false (Ran out of memory while building cycle collector graph), at z:/build/build/src/xpcom/base/nsCycleCollector.cpp:929

	(no longer active) Assignee
	Comment 17 • 7 years ago

(In reply to Carsten Book [:Tomcat] from comment #15)
> sorry had to back this out for frequent r5 failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=105425298&repo=mozilla-
> inbound

Sigh, if you look at the stack trace for that crash, the presence of ICCRunnerFired() which is new code that was just added by bug 1367905 a few pushes before this one, and the fact that these oranges did not go away after this backout proves that it is bug 1367905 that needed to be backed out not this one.  I'm going to reland it again.  Please don't back it out without pinging me beforehand, this has been tested on try before.

	Pulsebot
	Comment 18 • 7 years ago

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae0d4245ce7
Dispatch the compartment-nuking part of WindowDestroyedEvent to the idle queue; r=smaug

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 7 years ago

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #17)
> (In reply to Carsten Book [:Tomcat] from comment #15)
> > sorry had to back this out for frequent r5 failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=105425298&repo=mozilla-
> > inbound
> 
> Sigh, if you look at the stack trace for that crash, the presence of
> ICCRunnerFired() which is new code that was just added by bug 1367905 a few
> pushes before this one
Some new code somewhere doesn't indicate anything.


, and the fact that these oranges did not go away
> after this backout proves that it is bug 1367905
That does

> that needed to be backed
> out not this one.  I'm going to reland it again.  Please don't back it out
> without pinging me beforehand, this has been tested on try before.
So did I test bug 1367905 and we both had issues with reftests.
I asked Tomcat to backout this, since when this landed, that was the first time those failures occurred.

	(no longer active) Assignee
	Comment 20 • 7 years ago

(In reply to Olli Pettay [:smaug] from comment #19)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #17)
> > (In reply to Carsten Book [:Tomcat] from comment #15)
> > > sorry had to back this out for frequent r5 failures like
> > > https://treeherder.mozilla.org/logviewer.html#?job_id=105425298&repo=mozilla-
> > > inbound
> > 
> > Sigh, if you look at the stack trace for that crash, the presence of
> > ICCRunnerFired() which is new code that was just added by bug 1367905 a few
> > pushes before this one
> Some new code somewhere doesn't indicate anything.

It's not the existence of the new code but what that bug was changing (i.e., causing CC/GC to be backed up and not free up memory frequently enough resulting in an OOM) which made it obvious to me that this bug couldn't have been at fault, since this bug technically only changes the timing of compartment nuking, not when we free memory.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 7 years ago

compartment nuking has very much to do with memory management ;)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7ae0d4245ce7