Closed Bug 1321903 Opened 3 years ago Closed 3 years ago

Refactor the timeout/interval management code out of nsGlobalWindow

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

This code now lives in TimeoutManager.  Note that this is a transition
state, the Timeout list management code also needs to be refactored out
later.

In order to simplify the lifetime management of the new class, its
lifetime is equal to the lifetime of its containing nsGlobalWindow.  In
a few places where we need to dispatch runnables to do asynchronous work
on this object, we hold the containing window alive to guarantee safety.

This patch also removes a bit of dead code that was left over from the
code removed in bug 1281793. *

* https://hg.mozilla.org/mozilla-central/rev/0ac748f4d677#l1.63
Blocks: 1312514
Assignee: nobody → ehsan
Just curious, could we easily reuse this code in workers too?
/me doesn't recall atm how different setTimeout/Interval code in workers is.
Comment on attachment 8816615 [details] [diff] [review]
Refactor the timeout/interval management code out of nsGlobalWindow

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

Thanks for doing this!  I think this definitely will help improve the maintainability of this code.  I would like to see a few more changes I mentioned below, though.

::: dom/base/nsGlobalWindow.cpp
@@ +9985,5 @@
>  
>    bool resetTimers = (!aIsBackground && AsOuter()->IsBackground());
>    nsPIDOMWindow::SetIsBackground(aIsBackground);
>    if (resetTimers) {
> +    FORWARD_TO_INNER_VOID(mTimeoutManager.ResetTimersForThrottleReduction, ());

Can we ever call this method without an inner window?  I feel like maybe we should MOZ_DIAGNOSTIC_ASSERT(mInnerWindow) and then just mInnerWindow->mTimeoutManager here.  Or possibly just include mInnerWindow in the resetTimers boolean calculation.

This is preferable to using the FORWARD macro because the tends to just generate warnings that no on ever audits if it doesn't hold true.  But it obscures other output from the browser which will make :erahm send you nastygrams.

::: dom/base/nsGlobalWindow.h
@@ +55,5 @@
>  #include "nsComponentManagerUtils.h"
>  #include "nsSize.h"
>  #include "nsCheapSets.h"
>  #include "mozilla/dom/ImageBitmapSource.h"
> +#include "mozilla/dom/TimeoutManager.h"

Please de-inline things so we don't need to expose TimeoutManager.h to half the tree.  One of the nice side-effects I'd like to achieve here is speeding up builds when we change timer code.

I think this can be done by holding a UniquePtr<TimeoutManager>, etc.

@@ +394,5 @@
>    virtual bool IsFrozen() const override;
>    virtual void SyncStateFromParentWindow();
>  
>    virtual nsresult FireDelayedDOMEvents() override;
> +  virtual bool IsRunningTimeout() override { return mTimeoutManager.IsRunningTimeout(); }

For similar reasons I'd really like to discourage putting code in the .h file unless there is a measured perf benefit.  In particular, this is a virtual method so there would be no benefit of putting this in the header.

@@ +1255,5 @@
>    already_AddRefed<nsWindowRoot> GetWindowRoot(mozilla::ErrorResult& aError);
>  
>    mozilla::dom::Performance* GetPerformance();
> +
> +  mozilla::dom::TimeoutManager& GetTimeoutManager()

If this is infallible I think it should just be called ::TimeoutManager() per our current naming guidelines.

@@ +1263,5 @@
> +
> +  // This method is used for asynchronous dispatches, where we need to have an
> +  // object that we can hold alive.  Since TimeoutManager isn't refcounted, we
> +  // need to dispatch this on the window object to ensure the window will stay
> +  // alive as long as we need it to.

Well, we can make the runnables hold a ref to the nsGlobalWindow that owns the mTimeoutManager.  Those runnables can still be defined in TimeoutManager.cpp and not require methods on nsGlobalWindow, though.

::: dom/base/nsPIDOMWindow.h
@@ +188,5 @@
>    }
>  
>    virtual bool IsRunningTimeout() = 0;
>  
> +  bool HasAudioContexts() const { return !mAudioContexts.IsEmpty(); }

Should this really exist on both inner and outer windows?  I assume we only track audio contexts on one of those.

Also, please move the implementation to nsGlobalWindow like:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4058

Again, I think we should avoid putting code in these headers that the world includes unless we have measured perf benefits.
Attachment #8816615 - Flags: review?(bkelly) → review-
Comment on attachment 8816615 [details] [diff] [review]
Refactor the timeout/interval management code out of nsGlobalWindow

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

::: dom/base/nsGlobalWindow.cpp
@@ +9985,5 @@
>  
>    bool resetTimers = (!aIsBackground && AsOuter()->IsBackground());
>    nsPIDOMWindow::SetIsBackground(aIsBackground);
>    if (resetTimers) {
> +    FORWARD_TO_INNER_VOID(mTimeoutManager.ResetTimersForThrottleReduction, ());

Can you please explain why this cannot be called when we don't have an inner window?  I'm specifically worried about the case where the inner gets unlinked by the CC, it nulls out mInnerWindow <http://searchfox.org/mozilla-central/rev/b9c9d257366379ac984622dbef38432a7fecd2e9/dom/base/nsGlobalWindow.cpp#2046> and then nsDocShell::SetIsActive() is called.

::: dom/base/nsGlobalWindow.h
@@ +55,5 @@
>  #include "nsComponentManagerUtils.h"
>  #include "nsSize.h"
>  #include "nsCheapSets.h"
>  #include "mozilla/dom/ImageBitmapSource.h"
> +#include "mozilla/dom/TimeoutManager.h"

Are you ok with paying the dynamic allocation cost to avoid the header inclusion?

@@ +394,5 @@
>    virtual bool IsFrozen() const override;
>    virtual void SyncStateFromParentWindow();
>  
>    virtual nsresult FireDelayedDOMEvents() override;
> +  virtual bool IsRunningTimeout() override { return mTimeoutManager.IsRunningTimeout(); }

FWIW the reason why I chose to include TimeoutManager.h here wasn't inlining this, but avoiding the dynamic allocation which seems unnecessary.  This function doesn't even need to be virtual, if its only caller <http://searchfox.org/mozilla-central/rev/b9c9d257366379ac984622dbef38432a7fecd2e9/layout/painting/ActiveLayerTracker.cpp#364> would just use nsGlobalWindow::Cast().  I was planning to devirtualize this as a follow-up.

@@ +1263,5 @@
> +
> +  // This method is used for asynchronous dispatches, where we need to have an
> +  // object that we can hold alive.  Since TimeoutManager isn't refcounted, we
> +  // need to dispatch this on the window object to ensure the window will stay
> +  // alive as long as we need it to.

In response to the first sentence, that's what the runnable already does.

In response to the second sentence, I'm not sure what you mean.  Do you mean defining an explicit class deriving from nsRunnable instead of using NewRunnableMethod?  Or something else?
Flags: needinfo?(bkelly)
Ugh, the comments didn't appear as I expected them to... Sorry about that.  Please look at them in Splinter, there they're next to your comments.  I'll respond in the bug the next time!
(In reply to :Ehsan Akhgari from comment #4)
> Can you please explain why this cannot be called when we don't have an inner
> window?  I'm specifically worried about the case where the inner gets
> unlinked by the CC, it nulls out mInnerWindow
> <http://searchfox.org/mozilla-central/rev/
> b9c9d257366379ac984622dbef38432a7fecd2e9/dom/base/nsGlobalWindow.cpp#2046>
> and then nsDocShell::SetIsActive() is called.

Fair enough.  I still don't like the use of FORWARD_TO_INNER, though.  It produces warnings which will unleash the "wrath of Rahm".

Maybe we could pull the GetCurrentInnerWindowInternal() out of the other branches in this method.  Maybe refactor it to something like:

void nsGlobalWindow::SetIsBackground(bool aIsBackground)
{
  MOZ_ASSERT(IsOuterWindow());

  bool resetTimers = (!aIsBackground && AsOuter()->IsBackground());
  nsPIDOMWindow::SetIsBackground(aIsBackground);

  if (aIsBackground) {
    return;
  }

  nsGlobalWindow* inner = GetCurrentInnerWindowInternal();
  if (!inner) {
    return;
  }

  if (resetTimers) {
    inner->mTimerManager.ResetTimersForThrottleReduction();
  }

  inner->UnthrottleIdleCallbackRequests();

#ifdef MOZ_GAMEPAD
  inner->SyncGamepadState();
#endif
}

> > +#include "mozilla/dom/TimeoutManager.h"
> 
> Are you ok with paying the dynamic allocation cost to avoid the header
> inclusion?

Yes.  I think that cost is minimal here compared to all the dynamic allocations that are done when creating a window.  The benefit we gain by being able to iterate on the complex timer code without triggering huge compiles outweighs that cost, IMO.

> 
> @@ +394,5 @@
> >    virtual bool IsFrozen() const override;
> >    virtual void SyncStateFromParentWindow();
> >  
> >    virtual nsresult FireDelayedDOMEvents() override;
> > +  virtual bool IsRunningTimeout() override { return mTimeoutManager.IsRunningTimeout(); }
> 
> FWIW the reason why I chose to include TimeoutManager.h here wasn't inlining
> this, but avoiding the dynamic allocation which seems unnecessary.  This
> function doesn't even need to be virtual, if its only caller
> <http://searchfox.org/mozilla-central/rev/
> b9c9d257366379ac984622dbef38432a7fecd2e9/layout/painting/ActiveLayerTracker.
> cpp#364> would just use nsGlobalWindow::Cast().  I was planning to
> devirtualize this as a follow-up.
> 
> @@ +1263,5 @@
> > +
> > +  // This method is used for asynchronous dispatches, where we need to have an
> > +  // object that we can hold alive.  Since TimeoutManager isn't refcounted, we
> > +  // need to dispatch this on the window object to ensure the window will stay
> > +  // alive as long as we need it to.
> 
> In response to the first sentence, that's what the runnable already does.
> 
> In response to the second sentence, I'm not sure what you mean.  Do you mean
> defining an explicit class deriving from nsRunnable instead of using
> NewRunnableMethod?  Or something else?

I mean having something like this in TimeoutManager.cpp:

  void
  TimeoutManager::CancelOrUpdateBackPressure(nsGlobalWindow* aWindow)
  {
    MOZ_ASSERT(aWindow == mWindow);
    // do stuff
  }

And then to schedule it:

  nsCOMPtr<nsIRunnable> r =
    NewNonOwningRunnableMethod<StorensRefPtrPassByPtr<nsGlobalWindow>>(this, mWindow);

This holds the owning window alive as an argument.  Or we could just make TimeoutManager ref-counted and make ~nsGlobalWindow() clear the TimeoutManager::mWindow.

I'd just like to contain all of the logic in the isolated classes if we can instead of bouncing through nsGlobalWindow.  I think that obfuscates things.
Flags: needinfo?(bkelly)
It turns out that FORWARD_INNER has an ever bigger problem in that it returns unconditionally in the end, which means that it's totally not OK to use here.  Luckily there was 1 test that was failing as a result of that early return.  I'll redo this function as you suggested.
This code now lives in TimeoutManager.  Note that this is a transition
state, the Timeout list management code also needs to be refactored out
later.

In order to simplify the lifetime management of the new class, its
lifetime is equal to the lifetime of its containing nsGlobalWindow.  In
a few places where we need to dispatch runnables to do asynchronous work
on this object, we hold the containing window alive to guarantee safety.

This patch also removes a bit of dead code that was left over from the
code removed in bug 1281793. *

* https://hg.mozilla.org/mozilla-central/rev/0ac748f4d677#l1.63
Attachment #8818269 - Flags: review?(bkelly)
Attachment #8816615 - Attachment is obsolete: true
Comment on attachment 8818269 [details] [diff] [review]
Refactor the timeout/interval management code out of nsGlobalWindow

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

Thanks for the changes!  I think the main thing I'd like to see tweaked is that we shouldn't really allow any of the timer stuff on outer windows.  So I think the TimeoutManager should only be allocated for inner windows. r=me with comments addressed.

Note, I largely assumed that the contents of TimeoutManager.cpp matched the code removed from nsGlobalWindow.cpp.

::: dom/base/IdleRequest.cpp
@@ +63,5 @@
>  nsresult
>  IdleRequest::SetTimeout(uint32_t aTimeout)
>  {
>    int32_t handle;
> +  auto win = nsGlobalWindow::Cast(mWindow);

If you move TimeoutManager() to nsPIDOMWindowInner you can remove the Cast() here.

@@ +126,5 @@
>  void
>  IdleRequest::CancelTimeout()
>  {
>    if (mTimeoutHandle.isSome()) {
> +    nsGlobalWindow::Cast(mWindow)->TimeoutManager().ClearTimeout(

And you should be able to remove the Cast() here.

::: dom/base/nsGlobalWindow.cpp
@@ +1238,5 @@
>      mHasSeenGamepadInput(false),
>  #endif
>      mNotifiedIDDestroyed(false),
>      mAllowScriptsToClose(false),
> +    mTimeoutManager(MakeUnique<mozilla::dom::TimeoutManager>(*this)),

I think we should only allocate this for inner windows.

@@ +3114,5 @@
>    // reference to the script context, allowing it to be deleted
>    // later. Meanwhile, keep our weak reference to the script object
>    // so that it can be retrieved later (until it is finalized by the JS GC).
>  
> +  NS_ASSERTION(!mTimeoutManager->HasTimeouts(), "Uh, outer window holds timeouts!");

If we don't allocate the TimeoutManager on outer windows then this invariant is enforced up the stack and we can drop the assertion here.

@@ +12098,5 @@
>  
> +bool
> +nsGlobalWindow::IsRunningTimeout()
> +{
> +  return mTimeoutManager->IsRunningTimeout();

Can you add a MOZ_ASSERT(IsInnerWindow()) here to document that its inner only?  In theory someone might try to call it from inside an nsGlobalWindow that is really an outer.

::: dom/base/nsGlobalWindow.h
@@ +1258,5 @@
>    mozilla::dom::Performance* GetPerformance();
> +
> +  mozilla::dom::TimeoutManager& TimeoutManager()
> +  {
> +    return *mTimeoutManager;

Can you move the TimeoutManager() accessor to nsPIDOMWindowInner?  We should never be able to set timers on an outer window.

If you also have an nsGlobalWindow::TimeoutManager(), please make sure to MOZ_DIAGNOSTIC_ASSERT(IsInnerWindow()).

Also, I still don't like inlined methods in headers, but I'll digress on that point. :-)
Attachment #8818269 - Flags: review?(bkelly) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9439982efdc3
Refactor the timeout/interval management code out of nsGlobalWindow; r=bkelly
https://hg.mozilla.org/mozilla-central/rev/9439982efdc3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.