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+

Horky
Reported 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.
Attachments
Fixing patch for review (1.00 KB, patch)
2014-10-19 19:46 PDT, Horky 		no flags
DetailsFormatted DiffDiff
Fixing patch for review (976 bytes, patch)
2014-10-19 21:35 PDT, Horky 		darin: review-
DetailsFormatted DiffDiff
Fixing patch for review (1.43 KB, patch)
2014-10-20 19:24 PDT, Horky 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Horky
Comment 1 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.
Horky
Comment 2 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.
Horky
Comment 3 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?
Darin Adler
Comment 4 2014-10-19 20:41:51 PDT
The correct way to ask for review is to set the review flag to ?, not to +.
WebKit Commit Bot
Comment 5 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.
Horky
Comment 6 2014-10-19 21:35:40 PDT
Created attachment 240098 [details] Fixing patch for review Corrected the review flag and coding style. Thanks.
Darin Adler
Comment 7 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.
Horky
Comment 8 2014-10-20 19:24:10 PDT
Created attachment 240176 [details] Fixing patch for review Thanks for Darin's Review!
Horky
Comment 9 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.
Csaba Osztrogonác
Comment 10 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.
Lucas Forschler
Comment 11 2019-02-06 09:18:50 PST
Mass move bugs into the DOM component.
Ahmad Saleem
Comment 12 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".
Note You need to log in before you can comment on or make changes to this bug.