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.
(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.
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.
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.
Created attachment 382374 [details] Patch
Created attachment 382379 [details] Patch
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.
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.
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.
Created attachment 382387 [details] Patch
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.
(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.
(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.
(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.
(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.
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
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
<rdar://problem/56783603>