Bug 137631

Summary: DOMTimer nesting level may not impact each other.
Product: WebKit Reporter: Horky <chenhao>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: ahmad.saleem792, ap, barraclough, bfulgham, cdumez, chenhao, commit-queue, darin, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Fixing patch for review
none
Fixing patch for review
darin: review-
Fixing patch for review darin: review+

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.
Comment 12 Ahmad Saleem 2022-07-30 07:40:30 PDT 
I updated test case from Comment 0 with HTTPS link and new images:

Link - https://jsfiddle.net/c10wr9y3/show

In Safari 15.6, clicking on "Goto Next Page" does not work at all while it works fro all other browsers and I get following in Console:

Refused to display 'https://webkit.org/' in a frame because it set 'X-Frame-Options' to 'SAMEORIGIN'.

I am not sure this is the behavior about this bug or not but this is my testing results, all other browsers allow to clicking "Goto Next Page" and then navigate by giving errors and upon return, they show "Timer" as "PAUSED".