bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Unify logic that sets Timeout mWhen and mTimeRemaining values

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8824542 [details] [diff] [review]
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
Attachment #8824531 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
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)
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
Created attachment 8824565 [details] [diff] [review]
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
Attachment #8824542 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
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 7

2 years ago
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 8

2 years ago
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+
(Assignee)

Comment 9

2 years ago
(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.
(Assignee)

Comment 10

2 years ago
Created attachment 8825183 [details] [diff] [review]
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
Attachment #8824542 - Attachment is obsolete: true
Attachment #8824565 - Attachment is obsolete: true
Attachment #8825183 - Flags: review+
(Assignee)

Updated

2 years ago
Blocks: 1329787

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc19f506f9fc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a year ago
Depends on: 1346426
You need to log in before you can comment on or make changes to this bug.