1329284 - Unify logic that sets Timeout mWhen and mTimeRemaining values

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

The Timeout mWhen and mTimeRemaining values need to be set carefully based on the state of the window.  Right now we have logic to do this spread out all over the place.  We should unify it in a single method so bugs like bug 1329006 don't happen again.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc533d4cf753e45f42873a26103d2a5b4aa8426c

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

One of my assertions was to restrictive.  Overall I am trying to lock down some of our invariants with this patch as well as just refactoring the code.

Lets try again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2ed9f1b344f483ffd999ac0093c4ebff6e94d27

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824542 [details] [diff] [review]
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

Ehsan, this patch moves the settings on mWhen and mTimeRemaining into the Timeout class.  We were setting these values in at least 3 places in TimeoutManager (and nsGlobalWindow) previously.  This was problematic because we need to be consistent in switching to mTimeRemaining when the window is frozen.

I also added some assertions to clarify some of the invariants in the code.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824542 [details] [diff] [review]
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

Ah, I missed another assertion I should have loosened.  Sorry!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

Once more, with feeling!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0b9ced09bdcbf08719baa114dcc2f9ba67d1f3c

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824565 [details] [diff] [review]
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

Please see previous comments for the explanation of the patch.

Comment 7

[(no longer active)]

Comment on attachment 8824542 [details] [diff] [review]
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

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

Most of the comments below are follow-up ideas, and it's OK if you don't do them here since this patch on its own improves things a lot!

::: dom/base/Timeout.cpp
@@ +115,5 @@
> +  // If we are frozen simply set mTimeRemaining to be the "time remaining" in
> +  // the timeout (i.e., the interval itself).  This will be used to create a
> +  // new mWhen time when the window is thawed.  The end effect is that time does
> +  // not appear to pass for frozen windows.
> +  if (mWindow && mWindow->IsFrozen()) {

It's crappy that we really have to deal with 3 cases here (the case where mWindow is null.)  However that case is really never going to happen and I think we can make it so relatively easily by making Timeout's ctor accept a window pointer and assert that it's null and make mWindow a const RefPtr so that it cannot be assigned (and thus reset to null.)  Then you could assume non-null-ness here.

@@ +140,5 @@
> +Timeout::When() const
> +{
> +  MOZ_DIAGNOSTIC_ASSERT(!mWhen.IsNull());
> +  // Note, mWindow->IsFrozen() can be true here.  The Freeze() method calls
> +  // When() to calculate the delay to populate mTimeRemaining.

:(

I *think* we can alleviate this by just calling TimeoutManager::Freeze() a bit earlier.  Can you please file a follow-up?

::: dom/base/Timeout.h
@@ +97,5 @@
>    nsCOMPtr<nsITimeoutHandler> mScriptHandler;
>  
>  private:
> +  // mWhen and mTimeRemaining can't be in a union, sadly, because they
> +  // have constructors.

These can be a mozilla::Variant!

::: dom/base/TimeoutManager.cpp
@@ +181,3 @@
>  
> +  TimeDuration delta = TimeDuration::FromMilliseconds(realInterval);
> +  timeout->SetWhenOrTimeRemaining(TimeStamp::Now(), delta);

Nice!

@@ +1073,5 @@
>        return;
>      }
>  
> +    // Set When() back to the time when the timer is supposed to fire.
> +    aTimeout->SetWhenOrTimeRemaining(now, aTimeout->TimeRemaining());

Can you add a MOZ_DIAGNOSTIC_ASSERT similar to the ones above here too?

Comment 8

[(no longer active)]

Comment on attachment 8824565 [details] [diff] [review]
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

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

r=me with the comments addressed.  I think all of them apply to this patch too.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #7)
> It's crappy that we really have to deal with 3 cases here (the case where
> mWindow is null.)  However that case is really never going to happen and I
> think we can make it so relatively easily by making Timeout's ctor accept a
> window pointer and assert that it's null and make mWindow a const RefPtr so
> that it cannot be assigned (and thus reset to null.)  Then you could assume
> non-null-ness here.

I did this because of the dummy timeouts having a nullptr window.  Now that I added a separate SetDummyWhen() method, though, I think I can assert mWindow here.

> > +  MOZ_DIAGNOSTIC_ASSERT(!mWhen.IsNull());
> > +  // Note, mWindow->IsFrozen() can be true here.  The Freeze() method calls
> > +  // When() to calculate the delay to populate mTimeRemaining.
> 
> :(
> 
> I *think* we can alleviate this by just calling TimeoutManager::Freeze() a
> bit earlier.  Can you please file a follow-up?

I would prefer not to do that.  I think it would complicate the nsGlobalWindow code and only gain us a small benefit of another assertion here.

> > +  // mWhen and mTimeRemaining can't be in a union, sadly, because they
> > +  // have constructors.
> 
> These can be a mozilla::Variant!

Yes, I think that's doable.  I'll file a follow-up.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

New try build since I added that additional assertion that mWindow is non-null in SetWhenOrTimeRemaining():

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db4cb0fabb2911889a94170111ceeab02a1fb9b4

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc19f506f9fc
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc19f506f9fc