Bug 203586 – Events with the same firing time can fire in the wrong order after page suspend and resume

• Status: ASSIGNED
• Component: WebCore Misc.

[Daniel Bates]

Created attachment 382223 [details]
Test

Open the attached test case. You should see all PASS messages. But you will see some FAILs. Reloading the page will show different timers failing each time OR you may get lucky (I haven't, yet) and see only PASS messages.

^^^ demonstrates that the insertion order of suspended timers are not preserved.

[Daniel Bates]

(In reply to Daniel Bates from comment #0)
> Created attachment 382223 [details]
> Test
> 
> Open the attached test case. You should see all PASS messages. But you will
> see some FAILs. Reloading the page will show different timers failing each
> time OR you may get lucky (I haven't, yet) and see only PASS messages.
> 
hah, I just got a perfect PASS with shipping. But on reload, I see some FAILs.

[Daniel Bates]

Created attachment 382239 [details]
Patch and layout test

This patch will not apply until <https://bugs.webkit.org/show_bug.cgi?id=203519> is landed.

[Daniel Bates]

Comment on attachment 382239 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=382239&action=review

> Source/WebCore/platform/Timer.cpp:275
> +    setNextFireTime(MonotonicTime::now() + nextFireInterval, insertionOrder);

This should be WTFMove()ed.

[Daniel Bates]

Created attachment 382374 [details]
Patch

[Daniel Bates]

Created attachment 382379 [details]
Patch

[Chris Dumez]

Comment on attachment 382379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382379&action=review

> Source/WebCore/dom/ActiveDOMObject.h:65
> +    virtual void suspend(ReasonForSuspension, MonotonicTime beginTime);

Passing a new time to the suspend() function for all ActiveDOMObjects even though it is meaningless for most of them, seems very unfortunate. There must be a less invasive way to fix this.

[Ryosuke Niwa]

Hm... I don't think we want to land this patch as is since I'm trying to integrate SuspendableTimer into WindowEventLoop / WorkerEventLoop, and once that's done then the suspendable timer's  ordering will kept by those Window/WorkerEventLoop objects.

[Chris Dumez]

Comment on attachment 382379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382379&action=review

> Source/WebCore/ChangeLog:9
> +            1. Patches SuspendableTimer so that it can save and restore the timer's insertion order

I personally think we should use a different container for ScriptExecutionContext's ActiveDOMObjects so that the iteration order is consistent. This would be a much more minimal patch and the ScriptExecutionContext is the only place that deals with a list of ActiveDOMObjects.

[Daniel Bates]

Created attachment 382387 [details]
Patch

[Chris Dumez]

Comment on attachment 382387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382387&action=review

> Source/WebCore/ChangeLog:8
> +        This patch does two things:

Previous review comments were not addressed.

[Daniel Bates]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 382379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382379&action=review
> 
> > Source/WebCore/dom/ActiveDOMObject.h:65
> > +    virtual void suspend(ReasonForSuspension, MonotonicTime beginTime);
> 
> Passing a new time to the suspend() function for all ActiveDOMObjects even
> though it is meaningless for most of them, seems very unfortunate. There
> must be a less invasive way to fix this.

Yes, I also solved using 

1. thread-local stored suspension begin time with a RAII object that you used whenever iterating over ActiveDOMObjects.

2. Per-SuspendableTimer suspension begin time.

3. Switching all unordered collections of active DOM/timers to be ordered.

I didn't like (1) because it was too subtle. You have to remember to set it before iterating over the unordered collection. I didn't like (3) because it wasn't enforceable: future new code may accidentally use an unordered collection of active DOM / timers and suspend/resume them. I did kinda like (2), but I felt it also suffered from the same subtlety as (1). I wound up at the current solution, which is very explicit.

[Daniel Bates]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 382379 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382379&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +            1. Patches SuspendableTimer so that it can save and restore the timer's insertion order
> 
> I personally think we should use a different container for
> ScriptExecutionContext's ActiveDOMObjects so that the iteration order is
> consistent. This would be a much more minimal patch and the
> ScriptExecutionContext is the only place that deals with a list of
> ActiveDOMObjects.

I thought about this doing just that, but rules it out because:

1. DOMTimerHoldingTank is another example of something that holds a list of timers that must suspend and resumes in the correct order. This was a primary motivator for this change.

2. Arbitrary code can suspend and resume individual timers and it seemed reasonable to make this work even though it was not the primary motivation of this change.

[Chris Dumez]

(In reply to Daniel Bates from comment #12)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 382379 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=382379&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +            1. Patches SuspendableTimer so that it can save and restore the timer's insertion order
> > 
> > I personally think we should use a different container for
> > ScriptExecutionContext's ActiveDOMObjects so that the iteration order is
> > consistent. This would be a much more minimal patch and the
> > ScriptExecutionContext is the only place that deals with a list of
> > ActiveDOMObjects.
> 
> I thought about this doing just that, but rules it out because:
> 
> 1. DOMTimerHoldingTank is another example of something that holds a list of
> timers that must suspend and resumes in the correct order. This was a
> primary motivator for this change.
> 
> 2. Arbitrary code can suspend and resume individual timers and it seemed
> reasonable to make this work even though it was not the primary motivation
> of this change.

I still do not think it is acceptable to add an extra parameter to ActiveDOMObject::suspend(). Maybe SuspendableTimer should not be an ActiveDOMObject at all. And as Ryosuke mentioned, he is about to rewrite SuspendableTimer to play nicely with the HTML5 event loop anyway.

[Chris Dumez]

(In reply to Daniel Bates from comment #12)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 382379 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=382379&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +            1. Patches SuspendableTimer so that it can save and restore the timer's insertion order
> > 
> > I personally think we should use a different container for
> > ScriptExecutionContext's ActiveDOMObjects so that the iteration order is
> > consistent. This would be a much more minimal patch and the
> > ScriptExecutionContext is the only place that deals with a list of
> > ActiveDOMObjects.
> 
> I thought about this doing just that, but rules it out because:
> 
> 1. DOMTimerHoldingTank is another example of something that holds a list of
> timers that must suspend and resumes in the correct order. This was a
> primary motivator for this change.

DOMTimerHoldingTank does not call suspend or resume on the timers. The scriptExecutionContext is in charge of suspending / resuming all active dom objects.

[EWS Watchlist]

Comment on attachment 382387 [details]
Patch

Attachment 382387 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13195544

New failing tests:
fast/dom/simultaneously-registered-timer-fire-order-after-navigation-back.html

[EWS Watchlist]

Created attachment 382460 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Radar WebKit Bug Importer]

<rdar://problem/56783603>