1270059 - make setTimeout() low priority during page load

Status: RESOLVED FIXED

(Core :: Performance, defect)

Description

[Cynthia Tang [:cynthiatang]]

In profiler result, we can see some heavy tasks come from setTimeOut() at the beginning of google doc loading. If the timer task could be handled later, then we can see the google doc toolbar much earlier. That could improve our user experience for google doc page loading time.


Profiler: https://cleopatra.io/#report=37b1b314384bfff047553f1995a62b608457f830&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A60024,%22end%22%3A60477%7D,%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A60118,%22end%22%3A60182%7D%5D&selection=0,3902,117,2

Comment 1

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

Interesting idea. Do we know what should/could be processed before setTimeout callbacks?

Comment 2

[Chris Peterson [:cpeterson]]

Ben, is this setTimeout bug still meaningful after all the timer work you did for Quantum?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Its really hard to say without another profile.  My setTimeout() changes will cause timeout callbacks to yield to other main thread work if they take more than 4ms of time.  This only really helps with large numbers of setTimeout() calls, though.  We don't interrupt individual long running callbacks.

In the comment 0 profile there is a single callback that seems to take a long time.  If that is still the case, then my changes won't help.

Its possible, though, that google docs has started using requestIdleCallback() for things that can delayed.  A new profile is probably necessary.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

The general algorithm here (not written in stone - more like in putty) is to push content setTimeouts/Intervals to a list processed off the Idle queue during pageload. On end of pageload we move them all back to the 'normal' queue. (Note that a Reschedule on that is probably still missing from the patch.)

Chrome does something similar in their scheduling; they push all timers to a different queue which they deprioritize during load. Overall there's a bunch of complexity around this in chrome, which I'm investigating for ideas. They will run timeouts during mainthred-idle periods.

We could use a new idle queue with different parameters. We could handle relative timeouts differently than in this patch...

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9037382 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

Looking for some early feedback on the idea of deprioritizing setTimeout firings during load, thanks!

Comment 8

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

Comment on attachment 9037382 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload


>+nsIEventTarget* TimeoutManager::IdleEventTarget() {
>+  return mWindow.EventTargetFor(TaskCategory::IdleCallback); // XXXX use a new one
>+}
er, hmm, does this actually end up returning anything useful or just leftover thing from Quantum DOM?
https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/dom/base/nsGlobalWindowInner.cpp#616-621 is how idle dispatching is done.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

Fixed the IdleDispatch (though probabaly I can do a better refactoring) and also fixed cancel of a deferred settimeout/interval.  Also now reschedules the normal executor as needed after releasing timeouts on load-complete.  I think a remaining problem is that the timeouts for subdocuments (iframes) are beind released when the subdocument loads, not when the top-level document loads.  bz: just looking for feedback on the plan; not necessarily the code (though you're free to look).  Still a WIP note!

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

It's definitely deferring most timeouts now; for facebook typically it doesn't run them until ~load -- or iframe load, I'm checking on that. Nationalgeographic has some idle time, so lots of timeouts run before load.

https://faraday.basschouten.com/mozilla/executionorder/externaltimeout0.html now paints a waffle (in low-resolution....) before running the timeout! But a second run fired Load and released the timeout before we painted, so we'll want to think if there's any adjustment we want to do (small delay after load, or trigger off something later in the pipeline, or leave it). https://perfht.ml/2TXti2z -- in the marker chart under settimeout, you can see that it deferred the timer by 7ms (and ran it from the idle callback) detail: https://perfht.ml/2TYsA54 Second run: https://perfht.ml/2U1kDMO

Loading buzzfeed shows that no timeouts ran off the idle queue (no idle time) -- however, there are a bunch of settimeout release markers, mostly before the final load event, which implies to me that they're due to loads of iframes and their timeouts are being released before the document load: https://perfht.ml/2TYtSwW So it may need additional work on where the load signal comes from.

There are separate markers for setTimeout -- deferred and ran off idle queue -- and setTimeout release -- moved back to head of normal queue after isLoading moves to false (with a MaybeSchedule pass to reset wait time if needed). I'll tweak the markers to make it easier to compare, as well as add a pref to flip this on and off

It builds clean across the board, though tp6 had some sort of problem - https://treeherder.mozilla.org/#/jobs?repo=try&author=rjesup%40wgate.com&selectedJob=222662253

Comment 11

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

Comment on attachment 9037472 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

>+  nsPIDOMWindowInner* inner = GetInnerWindow();
>+  if (READYSTATE_LOADING == rs || READYSTATE_INTERACTIVE == rs) {
>+    mLoadingTimeStamp = mozilla::TimeStamp::Now();
mLoadingtimeStamp is set with _LOADING and _INTERACTIVE

>   // At the time of loading start, we don't have timing object, record time.
>-  if (READYSTATE_LOADING == rs) {
>-    mLoadingTimeStamp = mozilla::TimeStamp::Now();
>-  }
But here only with _LOADING

Doesn't the change break all navigation timing handling?

 
>class TimeoutExecutor final : public nsIRunnable,
>                               public nsITimerCallback,
>                               public nsINamed {
>   TimeoutManager* mOwner;
>+  nsIEventTarget* mEventTarget;
>+  bool mIdleQueue;
mIsIdleQueue (when I saw the mIdleQueue usage elsewhere, I thought it was a queue.)


>+++ b/dom/base/Timeout.cpp
>@@ -15,17 +15,19 @@ namespace dom {
> Timeout::Timeout()
>     : mTimeoutId(0),
>       mFiringId(TimeoutManager::InvalidFiringId),
>       mPopupState(PopupBlocker::openAllowed),
>       mReason(Reason::eTimeoutOrInterval),
>       mNestingLevel(0),
>       mCleared(false),
>       mRunning(false),
>-      mIsInterval(false) {}
>+      mIsInterval(false) {
>+    mStart = TimeStamp();
TimeStamps' default ctor should work here.

mStart is a tad vague name. Is it more like... mSchedulingTime?

>+bool TimeoutManager::IsLoading() const {
>+  // A window is considered loading if:
>+  // * it hasn't fired the document load event
>+  // * it's not chrome
>+  // XXX Do we want to distinguish between before DCL and before load?
Do we need to?


>+void TimeoutManager::MoveIdleToActive() {
>+  int num = 0;
uint32_t


>+void TimeoutManager::ProcessIdleTimeouts() {
>+  TimeStamp now = TimeStamp::Now();
>+  RunTimeout(now, now, true); // XXX check
>+}
So nothing calls this method?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

Cleaned up a bunch of things.  Needs more asserts and testing, and probably tweaking.  Handles subdocuments by mirroring the loading start down from the top-level document.  Example buzzfeed profile, showing timeouts run off idle ('settimeout') and ones released when we finished loading ('settimeout release') - tags in the textmarkers could be better, and we may not leave them in anyways). Clearly delays LOTS of timeouts; I saw 60 being released on an idle timeout.... (though it will limit the time spent executing timeouts before going through the idle queue again, so that's ok - we may want to tune the max time down though)

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

uncomment a line I temporarily commented to test something

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

Updated for smaug's suggestions; added a dom.timeout.defer_during_load (defaults to true) which you can change live (no restart needed) to turn on and off the new settimeout behavior.  Also pushed a full try (without the pref and olli's changes) at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b22e05b3d82f07736c2ce6ff2e3c8d61b4119dd -- there's at least one UAF (https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222834596&repo=try&lineNumber=5592) and I haven't look at most of the rest (others I looked t were timeouts), so (no surprise) there's more work to do here.  Ehsan (and bz) - I'm more interested in your thoughts on the strategy here, though I'm happy to have feedback on the implementation

Comment 15

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

One thing need to be careful at is that if page loading takes forever, as it may, we probably don't want to slow down timers too long time.

And, need to ensure timers get still run before idle queue, since browser uses idle queue for several heavy operations, like GC and CC and object deletion.
As we discussed on IRC, perhaps we could add a new queue to the main thread, something which gets handled after normal priority queue but before idle queue..

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Well, even when it does take forever, the timeouts will still run off idle time on mainthread - but they may be delayed more than normal. We could put a max time of say 60 or 120 seconds on the delaying period, or perhaps variable based on <something> - bandwidth, device type (mobile vs desktop), activity, etc. We could also put a max time on how long a timeout will be delayed, though I'm less in love with that idea.

And yes, we should get this ahead of generic idle queue users.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

Much cleaner on Try now; mostly wpt1 and Reftest 'C' (and tp6)

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Nathan, bz - a ping for your feedback on the approach here. The general idea is to address some of the issues around scheduling during a load, in particular scheduling of setTimeout calls - we've found that part of the difference between us and chrome is that often sites (or ads on sites) schedule a lot of timeouts that we fire before the load finishes - and chrome fires after it finishes (or much later in the load sequence, after all the important parts of the page have rendered). Chrome purposely deprioritizes timeouts during load. From a webcompat point-of-view, we're not required to run them at any particular time (witness what we do with background tabs, etc).

When they run before load, they not only can be long, but they can also start loads of more resources which end up blocking the load event (since they end up in the loadgroup). I've also toyed with making resources loaded from timeouts to be put in a separate loadgroup, so they don't block load from firing. That generally wouldn't affect the appearance of the page in most cases (except where the load event itself does), but we'd turn off the throbber earlier and metrics would show the page as loaded earlier. There's justification (if not compliant with the spec) in that the timeout could have run after the load event, and if so the resources wouldn't have blocked the load event.

I have not done anything along these lines so far; this bug is all about deferring timeouts during load, and only running them when we process the idle queue. (We'll probably run timeouts ahead of any other idle runnables, like CC, etc, but the patch doesn't do this yet).

Thanks for any feedback, or thoughts on heuristics here. (For example, we could be more aggressive at running settimeout's for 0 or not defer them at all.)

Comment 19

[(no longer active)]

Comment on attachment 9037814 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

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

I think the general approach of lowering the priority of setTimeout() callbacks during high priority events such as pageloads makes a lot of sense so in general I think your patch is on the right track.  However I think it could improve in two possible ways:

1. I think the existing idle queue isn't a great place to add timeout handlers to.  Like smaug mentioned in comment 15, there may be other events in the idle queue before the timeout handlers which can be really expensive to run, and that is kind of the point of the idle queue.  I think we really want to add a new queue to the main thread called timer queue specifically for the purpose of storing low-priority timer handlers and nothing else and dispatch the timers during page load time to that queue instead of the idle queue.  Once we have that, there is no need to dispatch to that queue with a fixed timeout really, and we can get the best of both worlds, timeout handlers will have lower priority than all main event queue events during page loads and they won't be starved by the idle queue tasks.  We probably want to make sure that the timer queue is always serviced in order to avoid having to deal with the issues that not doing so would cause in the layers above IMO...

2. I think it would be nice if your patch didn't tie the lowering of the priority of timeouts merely to pageloads.  See this document from the Blink Scheduler docs if you haven't already <https://docs.google.com/document/d/163ow-1wjd6L0rAN3V_U6t12eqVkq4mXDDjVaA4OuvCA/edit> (note that it is rather old and possibly a bit out of date).  Blink lowers the priority of timeouts in several cases, for example during pageload, touch events, scrolling, etc.  I think we would want to also move towards a world where we can dynamically adjust the priority of timeouts as we enter various "use cases" (to borrow from Blink's scheduler vocabulary) in order to ensure that timeout handlers don't get the opportunity to jank the main thread during the times at which we expect to want to run code there.  Page load seems to be the first of these class of use cases, and it seems that with just a little bit of reworking of your patch you could for example tell the TimeoutManager that it's now time to enter low-priority mode etc. rather than the page is now loading.  I think that would be a more future-proof way of constructing this patch (even if the final outcome would be exactly the same functionality-wise.)

Thanks a lot for doing this work, I think the positive impact of this would be something users would appreciate!

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

1. I think the existing idle queue isn't a great place to add timeout
   handlers to. Like smaug mentioned in comment 15, there may be other events
   in the idle queue before the timeout handlers which can be really expensive
   to run, and that is kind of the point of the idle queue.

Agreed, and I said something similar in comment 18. This is a starting point/WIP/proof-of-concept before doing the other corollary patches.

2. I think it would be nice if your patch didn't tie the lowering of the
   priority of timeouts merely to pageloads. See this document from the Blink
   Scheduler docs if you haven't already
   <https://docs.google.com/document/d/163ow-
   1wjd6L0rAN3V_U6t12eqVkq4mXDDjVaA4OuvCA/edit> (note that it is rather old and
   possibly a bit out of date). Blink lowers the priority of timeouts in
   several cases, for example during pageload, touch events, scrolling, etc.

Yes; I started working on this patch after investigating Blink's timer behavior (which we could see was different in profiles). Once we have this, we can look at other inputs/controls; right now we just have inputs into the TimeoutManager; it decides how to schedule. We can certainly add more inputs - or move control of that outside of TimeoutManager as you suggest; that's largely a detail of where we want the scheduling control to live.

Thanks a lot for doing this work, I think the positive impact of this would
be something users would appreciate!

:-) thanks

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 9037814 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

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

This all looks reasonable to me, insofar as this is code that I'm not really responsible for.

I am not super-excited about the possibility of adding more code to PrioritizedEventQueue, but I also don't see how to put some of the logic in SchedulerGroup/TabGroup/DocGroup or similar.

::: dom/base/TimeoutManager.h
@@ +127,5 @@
>  
>    TimeDuration MinSchedulingDelay() const;
>  
> +  nsresult MaybeSchedule(RefPtr<TimeoutExecutor>& aExecutor,
> +                         const TimeStamp& aWhen,

There's this weird asymmetry in the patch where this function has been seemingly generalized by taking a TimeoutExecutor, but it's only ever used with mExecutor, never with the idle executor.  I don't think that's a good idea; afaict, it's wrong to use it with mIdleExecutor.  Can we change the prototype here and/or change the name?

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

I am not super-excited about the possibility of adding more code to
PrioritizedEventQueue, but I also don't see how to put some of the logic in
SchedulerGroup/TabGroup/DocGroup or similar.

Right; I think it has to live there somehow, and having a single prioritized queue and doing priority inserts doesn't seem like a great idea.

• nsresult MaybeSchedule(RefPtr<TimeoutExecutor>& aExecutor,

•                     const TimeStamp& aWhen,
  

There's this weird asymmetry in the patch where this function has been
seemingly generalized by taking a TimeoutExecutor, but it's only ever used
with mExecutor, never with the idle executor. I don't think that's a good
idea; afaict, it's wrong to use it with mIdleExecutor. Can we change the
prototype here and/or change the name?

Yeah, originally I had it handling both executors, but I don't want to update the budget for idleexecutor when we call MaybeSchedule. I can undo the API change to that method. Alternatively I could keep passing in the executor, and skip UpdateBudget if it's the idle queue. Or pass a boolean in and let MaybeScedule decide. Or have two MaybeSchedule methods,which imply the queue.

Personally, for now I'm just reverting the API change to MaybeSchedule.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

Incorporated froyd's comments, removed unneeded event target getter

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

small fix

Comment 25

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

Comment on attachment 9038629 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

># HG changeset patch
># User Randell Jesup <[e-mail]>
># Date 1548273415 18000
>#      Wed Jan 23 14:56:55 2019 -0500
># Node ID 34ff73c794b7f7cc329e1ab130e21e9674ed2586
># Parent  af55f215496f884325bbec9a5722b62c1c92c97f
>Bug 1270059: WIP patch to defer setTimeout()s in content during pageload
>
>diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
>--- a/dom/base/Document.cpp
>+++ b/dom/base/Document.cpp
>@@ -8100,24 +8100,58 @@ nsresult Document::CloneDocHelper(Docume
>   // State from Document
>   clone->mType = mType;
>   clone->mXMLDeclarationBits = mXMLDeclarationBits;
>   clone->mBaseTarget = mBaseTarget;
> 
>   return NS_OK;
> }
> 
>+static bool SetLoadingInSubDocument(Document* aDocument, void* aData) {
>+  nsPIDOMWindowInner* inner = aDocument->GetInnerWindow();
>+  if (inner) {
>+    inner->SetLoading(*(static_cast<bool*>(aData)));
>+  }
>+  return true;
>+}
>+
> void Document::SetReadyStateInternal(ReadyState rs) {
>-  mReadyState = rs;
>   if (rs == READYSTATE_UNINITIALIZED) {
>     // Transition back to uninitialized happens only to keep assertions happy
>     // right before readyState transitions to something else. Make this
>     // transition undetectable by Web content.
>-    return;
>-  }
>+    mReadyState = rs;
>+    return;
>+  }
>+
>+  // Mirror the top-level loading state down to all subdocuments
>+  bool set_state = true;
>+  bool new_state = false;
state of what? These variable should have names stating what they are for. So, wasLoading (or was_loading) and isLoading (is_loading) ?

>+  if (READYSTATE_LOADING == rs) {
>+    mLoadingTimeStamp = mozilla::TimeStamp::Now();
>+    new_state = true;
So, if one does document.open() we update state again. I don't think we want that.  It is not really a page load.


>+  } else if (READYSTATE_INTERACTIVE == rs) {
>+    new_state = true;
>+  } else if (mReadyState == READYSTATE_LOADING ||
>+             mReadyState == READYSTATE_INTERACTIVE) {
>+    new_state = false;
Why mReadyState == READYSTATE_LOADING causes new state to be false?


>+  } else {
>+    set_state = false;
>+  }
>+  if (IsTopLevelContentDocument()) {
>+    if (set_state && StaticPrefs::dom_timeout_defer_during_load()) {
>+      nsPIDOMWindowInner* inner = GetInnerWindow();
>+      if (inner) {
>+        inner->SetLoading(new_state);
>+      }
>+      EnumerateSubDocuments(SetLoadingInSubDocument, &new_state);


I don't understand how this stuff works when new iframes are added by scripts during page load.
How do they get loading flag set?
var ifr = document.createElement("iframe");
document.body.appendChild(ifr);
ifr.contentWindow.setTimeout("run some script here");
should still be deferred when top level page loads, I think.



>   enum class Reason : uint8_t {
>     eTimeoutOrInterval,
>     eIdleCallbackTimeout,
>+    eIdleWhileLoading,
I'd prefer some a tad more descriptive name.
eIdleTimeoutOrIntervalWhileLoading 
or something. It is long, but hints that it is about Timeout/Interval, not about the idle callbacks. 

> 
>+  // Time submitted
>+  TimeStamp mSubmitTime;
The comment doesn't really say what the member variable means. Want to clarify a bit.



>+  nsresult rv;
>+  RefPtr<TimeoutExecutor> runnable(this);
You seem to use runnable only when mIsIdleQueue is true. Move inside if() {} ?

>+  if (mIsIdleQueue) {
>+    // We can't just Dispatch() to the Idle eventtarget, since that doesn't
>+    // wrap it in an IdleRunnableWrapper
I don't know what this comment means. There is no idle eventtarget.



>-TimeoutExecutor::TimeoutExecutor(TimeoutManager* aOwner)
>-    : mOwner(aOwner), mMode(Mode::None) {
>+TimeoutExecutor::TimeoutExecutor(TimeoutManager* aOwner, bool aIsIdleQueue,
>+                                 uint32_t aMaxIdleDeferMS)
>+    : mOwner(aOwner),
>+      mIsIdleQueue(aIsIdleQueue),
>+      mMaxIdleDeferMS(aMaxIdleDeferMS),
>+      mMode(Mode::None) {
>   MOZ_DIAGNOSTIC_ASSERT(mOwner);
>+  mEventTarget =
>+      aIsIdleQueue ? mOwner->IdleEventTarget() : mOwner->EventTarget();
Is something missing from the patch. I don't see IdleEventTarget on TimeoutManager
Or is some other bug adding IdleEventTarget

>+  // Note: not related to IsLoading.  Set to false if there's an error, etc.
>+  void SetLoading(bool aIsLoading);
>+
Since this is so easy to confuse with IsLoading, could we perhaps use different name.
Like... AllowDeferredTimeoutHandling or some such.





>diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
>--- a/modules/libpref/init/all.js
>+++ b/modules/libpref/init/all.js
>@@ -1335,16 +1335,20 @@ pref("dom.timeout.foreground_budget_rege
> // Maximum value (in ms) for the background budget. Only valid for
> // values greater than 0.
> pref("dom.timeout.foreground_throttling_max_budget", -1);
> // The maximum amount a timeout can be delayed by budget throttling
> pref("dom.timeout.budget_throttling_max_delay", 15000);
> // Turn on budget throttling by default
> pref("dom.timeout.enable_budget_timer_throttling", true);
> 
>+// Deprioritize timeouts while loading
>+pref("dom.timeout.defer_during_load", true);
>+pref("dom.timeout.max_idle_defer", 10000);
>+
Please using StaticPrefList, also for the max_idle_defer. It is easier and faster to use.
I don't see reason to put anything to all.js



We need some tests ensuring that setTimeout(,0) set during page load fires before setTimeout(,0) set during load event firing. And other similar things.

Comment 26

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

Comment on attachment 9038722 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

I think my comments for the previous patch apply here too, except the use of IdleEventTarget()

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Attachment: interdiffs for review comment changes

interdiffs for review changes

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

merged with review comments

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix timeout ordering when releasing timeouts on Load

our tests include settimeout(..., 0) ordering, and were intermittently failing.  Simple fix

Comment 30

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

Comment on attachment 9038980 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

>+++ b/dom/base/Document.cpp
>@@ -1290,16 +1290,17 @@ Document::Document(const char* aContentT
> #endif
>       mHasDelayedRefreshEvent(false),
>       mPendingFullscreenRequests(0),
>       mXMLDeclarationBits(0),
>       mOnloadBlockCount(0),
>       mAsyncOnloadBlockCount(0),
>       mCompatMode(eCompatibility_FullStandards),
>       mReadyState(ReadyState::READYSTATE_UNINITIALIZED),
>+      mParentIsLoading(false),
Call the variable mAncestorIsLoading


>+void Document::SetParentLoading(bool aParentIsLoading) {
And this could be then SetAncestorLoading


> nsresult TimeoutExecutor::ScheduleImmediate(const TimeStamp& aDeadline,
>                                             const TimeStamp& aNow) {
>   MOZ_DIAGNOSTIC_ASSERT(mDeadline.IsNull());
>   MOZ_DIAGNOSTIC_ASSERT(mMode == Mode::None);
>   MOZ_DIAGNOSTIC_ASSERT(aDeadline <= (aNow + mAllowedEarlyFiringTime));
> 
>-  nsresult rv =
>-      mOwner->EventTarget()->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
>+  nsresult rv;
>+  if (mIsIdleQueue) {
>+    RefPtr<TimeoutExecutor> runnable(this);
>+    MOZ_LOG(gTimeoutLog, LogLevel::Debug, ("Starting IdleDispatch runnable"));
>+    rv = NS_IdleDispatchToCurrentThread(runnable.forget(), mMaxIdleDeferMS,
>+                                        EventQueuePriority::DeferredTimers);
I guess the exact syntax here will depend on the DeferredTimers patch, but the change will be trivial.

>   if (!mTimer) {
>-    mTimer = NS_NewTimer(mOwner->EventTarget());
>+    mTimer = NS_NewTimer(mEventTarget);
This looks like unneeded change. We can just keep using mOwner->EventTarget().
No need to add mEventTarget member variable


>+TimeoutExecutor::TimeoutExecutor(TimeoutManager* aOwner, bool aIsIdleQueue,
>+                                 uint32_t aMaxIdleDeferMS)
>+    : mOwner(aOwner),
>+      mIsIdleQueue(aIsIdleQueue),
>+      mMaxIdleDeferMS(aMaxIdleDeferMS),
>+      mMode(Mode::None) {
>   MOZ_DIAGNOSTIC_ASSERT(mOwner);
>+  mEventTarget = mOwner->EventTarget();
So this mEventTarget is useless


>+void TimeoutManager::SetLoading(bool value) {
>+  // When moving from loading to non-loading, we may need to
>+  // reschedule any existing timeouts from the idle timeout queue
>+  // to the normal queue.
>+  MOZ_LOG(gTimeoutLog, LogLevel::Debug, ("%p: SetLoading(%d)", this, value));
>+  if (mIsLoading && !value) {
>+    MoveIdleToActive();
>+  }
>+  // We don't move existing timeouts to the idle queue if we move to
>+  // loading (XXX if that ever happens?).
Ok, so this comments hints about the model I was expecting, that timeouts go to idle queue during load.
But apparently they go to idle queue only if timer fires during load. Please clarify the comment here.



>+        // MOZ_RELEASE_ASSERT(timeout->When() <= (TimeStamp::Now()));
>+        mIdleTimeouts.InsertBack(timeout);
>+        int num = 0;
>+        for (Timeout* t = mIdleTimeouts.GetFirst(); t != nullptr;
>+             t = t->getNext()) {
>+          num++;
>+        }
We don't want to itarate through idle timouts always, even when logging isn't enabled.
And don't use int, but uint32_t or so.



>diff --git a/modules/libpref/init/StaticPrefList.h b/modules/libpref/init/StaticPrefList.h
...
>+
>+// Maximum deferral time for setTimeout/Interval
... in seconds



Minor things, but I'd like to take a quick look still, since this isn't too simple stuff.

I'd really like to see some test. Something which tests both the pref on and off how timers work during/around page load. Something which has timers also in iframes.
We need such for tracking possible regressions in the future etc.

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Attachment: review.patch

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to defer setTimeout()s in content during pageload

* * *
Bug 1270059: interdiffs for review comment changes
* * *
Bug 1270059: Fix timeout ordering when releasing timeouts on Load r=smaug
* * *
Bug 1270059: review response interdiffs

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Assume the IdleExecutor should always ask for DeferredTimers idle callbacks

Nothing should ever be in the mIdleTimeouts list if it wasn't going to fire immediately at the point where we move it from mTimeouts.  Should resolve the wpt test oranges

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix talos to handle hero elements that are gated on timeouts

I'll take an r+ from anyone on the talos team; I want to land this today or this weekend

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Green try apparently (some tests still running): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eed805e2f3f12db0551ac88cb90192c8e6fd9f8
And talos with --rebuild 5: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d624f892c917f19d4dc432c03a07f1dab994ab7

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix talos to handle hero elements that are gated on timeouts

tweaked max extra delay to 5s; only delays if the hero element is late showing up (which appears to only be on tp6-google). Green on try (see above)

Comment 37

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 9039306 [details] [diff] [review]
fix talos to handle hero elements that are gated on timeouts

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

This makes sense and looks good to me. I’m still not very familiar with this code, so I’ll leave the other review requests open in case you prefer to wait for them before landing.

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 9039306 [details] [diff] [review]
fix talos to handle hero elements that are gated on timeouts

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

any thoughts on making similar changes to raptor?

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8594d21c51
Defer setTimeout/Intervals()s in content during pageload r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec1a6963a9c
fix talos to handle hero elements that are gated on timeouts r=davehunt,jmaher

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cec0a3a47cdb
ESLint fix r=bustage

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

jmaher - I'm still getting intermittent oranges on Talos TP6 in
https://hg.mozilla.org/integration/mozilla-inbound/rev/cec0a3a47cdb
It was green on my trys. See: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d624f892c917f19d4dc432c03a07f1dab994ab7

Thoughts? Just increase the time we wait for the Hero? (perhaps it's another item, not the hero, which wasn't done by load time?) Runs fine locally now.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/925fed5e69e7
Disable test_ext_async_clipboard.html on android only r=nika

Comment 43

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7f8594d21c51
https://hg.mozilla.org/mozilla-central/rev/eec1a6963a9c
https://hg.mozilla.org/mozilla-central/rev/cec0a3a47cdb
https://hg.mozilla.org/mozilla-central/rev/925fed5e69e7

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Randell, fyi
Coverity thinks that a part of the code could be simplified

dead_error_condition: The condition aProcessIdle must be true.

https://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#924

as we do if (aProcessIdle) { a few lines above:
https://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#917

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Thanks, I'll add it to some of my cleanup (likely I'll remove the outer if() and add markers for all setTimeouts)

Comment 46

[Joel Maher ( :jmaher ) (UTC -8)]

as discussed on irc- this is only affecting tp6 stylo-threads=1 which is a test we are not planning to run long term on talos, so disabling the test is fine (in this case tp6-google)

Comment 47

[Robert Wood [:rwood]]

Comment on attachment 9039306 [details] [diff] [review]
fix talos to handle hero elements that are gated on timeouts

We are planning to turn off Talos tp6 tests completely in favour of Raptor (there is only one remaining Talos tp6 job in production - stylo threads - see Bug 1504251).

:jesup, is this hero element change something that you are planning to make in Raptor also?

Comment 48

[Sylvestre Ledru [:Sylvestre]]

please create a separate bug for that :)

Comment 49

[Randell Jesup [:jesup] (needinfo me)]

:jesup, is this hero element change something that you are planning to make
in Raptor also?

Only if we see oranges from it missing the hero; given how raptor works (and that it measures ttfi, which requires more time) I doubt it will.

Comment 50

[Kohei Yoshino [:kohei]]

Posted site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2019/settimeout-and-setinterval-are-now-deferred-during-page-load/ (Thanks for the heads-up, Ehsan!)

Comment 51

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you suggest a release note? Is there going to be any sort of blog post describing this that we can link to?