Closed Bug 1329284 Opened 3 years ago Closed 3 years ago

Unify logic that sets Timeout mWhen and mTimeRemaining values

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
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
Attachment #8824531 - Attachment is obsolete: true
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.
Attachment #8824542 - Flags: review?(ehsan)
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!
Attachment #8824542 - Flags: review?(ehsan)
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.
Attachment #8824565 - Attachment description: efactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan → Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan
Attachment #8824565 - Flags: review?(ehsan)
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?
Attachment #8824542 - Attachment is obsolete: false
Attachment #8824542 - Flags: feedback+
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.
Attachment #8824565 - Flags: review?(ehsan) → review+
(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.
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
Attachment #8824542 - Attachment is obsolete: true
Attachment #8824565 - Attachment is obsolete: true
Attachment #8825183 - Flags: review+
Blocks: 1329787
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc19f506f9fc
Refactor DOM timeout to set mWhen/mTimeRemaining from one place. r=ehsan
https://hg.mozilla.org/mozilla-central/rev/bc19f506f9fc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1346426
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.