Bug 137631 - DOMTimer nesting level may not impact each other.
Summary: DOMTimer nesting level may not impact each other.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-10 18:13 PDT by Horky
Modified: 2019-02-06 09:18 PST (History)
4 users (show)

See Also:


Attachments
Fixing patch for review (1.00 KB, patch)
2014-10-19 19:46 PDT, Horky
no flags Details | Formatted Diff | Diff
Fixing patch for review (976 bytes, patch)
2014-10-19 21:35 PDT, Horky
darin: review-
Details | Formatted Diff | Diff
Fixing patch for review (1.43 KB, patch)
2014-10-20 19:24 PDT, Horky
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Horky 2014-10-10 18:13:56 PDT
First, there is a long story which come from:
   https://bugs.webkit.org/show_bug.cgi?id=42861

Current situation is the timer could forward the gesture state only if the nesting level is zero. The problem is the nesting level is managed in the Document (ScriptExecutionContext), that means all timers in the same document share one nesting level. If one repeat timer was launched, all other timers after that could not forward the gesture state, due to all of them has a non-zero nesting level.

Below is my testing page, three images at the end of page could delay the onload event. Then follow below steps to reproduce what I met:
1. Click "Start repeating timer" first
2. After the text change to "Running...", click "Goto next page"
3. When WebKit page come out, click back button.

The expected page is the testing page, but it moves to the page before it, because the NavigationScheduler::mustLockBackForwardList refused the creation of new history item.

 <html>
 <head>
 </head>
 <body>
 <input type="button" value="Goto next page" onclick="gotoNextPage();">
 <input type="button" value="Start repeating timer" onclick="startRepeatingTimer();">
 <p>
 <div id="Timer">Paused</div>
 </p>
 <script type="text/javascript">

 function gotoNextPage(){
    setTimeout(function(){location.href="http://www.webkit.org/";},300);
 }

 function startRepeatingTimer(){
   setInterval(function(){document.getElementById("Timer").innerHTML="Running...";},500);
 }
 </script>

 <img src="http://therecoveringpolitician.com/wp-content/uploads/2013/01/moderate.jpg" width="640" height="480">
 <img src="http://cullogo.com/full/wallpapers-high-resolution-widescreen-hd-pk.jpg" width="640" height="480">
 <img src="http://www.highresolutionwallpapers.net/wallpapers/autumn-1680x1050.jpg" width="640" height="480">
 </body>
 </html>

According to Web App APIs defintion(http://www.w3.org/html/wg/drafts/html/CR/webappapis.html#timer-nesting-level), the nesting level of repeating timer will be increased in the loop. I doubt that the nesting level should be owned by the timer itself, do not share with others. Just as described in the definition, that is task's nesting level.
Comment 1 Horky 2014-10-10 18:22:03 PDT
I do not prefer to clear the nesting level before the early return in DOMTimer::fired, some algorithms rely on it. Maybe moving nesting level from Document to DOMTimer is a optional solution.
Comment 2 Horky 2014-10-15 23:47:06 PDT
(In reply to comment #1)
> I do not prefer to clear the nesting level before the early return in DOMTimer::fired, some algorithms rely on it. Maybe moving nesting level from Document to DOMTimer is a optional solution.

I miss-understood that, the DOMTimer already has its own nesting level. So that is reasonable to clear the nesting level of script execution context.
Comment 3 Horky 2014-10-19 19:46:54 PDT
Created attachment 240093 [details]
Fixing patch for review

I'd like to submit one patch to reset the nesting level in the script execution context, would you please help to review it?
Comment 4 Darin Adler 2014-10-19 20:41:51 PDT
The correct way to ask for review is to set the review flag to ?, not to +.
Comment 5 WebKit Commit Bot 2014-10-19 20:43:14 PDT
Attachment 240093 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.cpp:210:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/page/DOMTimer.cpp:211:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Horky 2014-10-19 21:35:40 PDT
Created attachment 240098 [details]
Fixing patch for review

Corrected the review flag and coding style. Thanks.
Comment 7 Darin Adler 2014-10-20 09:53:10 PDT
Comment on attachment 240098 [details]
Fixing patch for review

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

Looks good. A few small issues.

A larger issues is that we require a regression test with every bug fix. Can we construct one that demonstrates this bug? If we cannot, then the change log should include information about why we were not able to construct a regression test.

> Source/WebCore/ChangeLog:6
> +        Unreviewed.

This needs to include the Reviewed by NOBODY (OOPS!) line so the patch management system can add the name of the reviewer here.

> Source/WebCore/ChangeLog:9
> +        (WebCore::DOMTimer::fired):

This needs to describe the change and explain why it’s correct.

> Source/WebCore/page/DOMTimer.cpp:211
> +

I suggest omitting this blank line.
Comment 8 Horky 2014-10-20 19:24:10 PDT
Created attachment 240176 [details]
Fixing patch for review

Thanks for Darin's Review!
Comment 9 Horky 2014-10-20 19:27:18 PDT
(In reply to comment #7)
> Comment on attachment 240098 [details]
> Fixing patch for review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240098&action=review
> 
> Looks good. A few small issues.
> 
> A larger issues is that we require a regression test with every bug fix. Can
> we construct one that demonstrates this bug? If we cannot, then the change
> log should include information about why we were not able to construct a
> regression test.
> 
> > Source/WebCore/ChangeLog:6
> > +        Unreviewed.
> 
> This needs to include the Reviewed by NOBODY (OOPS!) line so the patch
> management system can add the name of the reviewer here.
> 
> > Source/WebCore/ChangeLog:9
> > +        (WebCore::DOMTimer::fired):
> 
> This needs to describe the change and explain why it’s correct.
> 
> > Source/WebCore/page/DOMTimer.cpp:211
> > +
> 
> I suggest omitting this blank line.

Thanks! I updated it again.
Comment 10 Csaba Osztrogonác 2015-09-14 11:13:36 PDT
Comment on attachment 240093 [details]
Fixing patch for review

Cleared review+ from obsolete attachment 240093 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 Lucas Forschler 2019-02-06 09:18:50 PST
Mass move bugs into the DOM component.