Bug 176017

Summary: Speedometer 2.0: jQuery test fails occasionally
Product: WebKit Reporter: Shiyu Zhang <shiyu.zhang>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: addyo, buildbot, cdumez, commit-queue, lforschler, mathias, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176142
Bug Depends on:    
Bug Blocks: 172339    
Attachments:
Description Flags
A possible fix for jQuery test
none
Patch none

Shiyu Zhang
Reported 2017-08-28 01:49:12 PDT
Created attachment 319169 [details] A possible fix for jQuery test Speedometer 2.0 crashes at jQuery test with high possibility on my laptop (i5-5300U, win8, Chrome R60). Currently, jQuery test generates the DOM tree in its index.html and loads app.js, which binds events to the DOM nodes, at the end of the index.html. Speedometer framework starts "Adding items" action after the "new-todo" DOM node is ready. In jQuery test, there's a chance that Speedometer starts "Adding items" as soon as the "new-todo" node is ready while app.js has not been loaded completely and no event is binded to the DOM node yet. A possible fix patch is in the attachment.
Attachments
A possible fix for jQuery test (1.42 KB, patch)
2017-08-28 01:49 PDT, Shiyu Zhang 		no flags
DetailsFormatted DiffDiff
Patch (2.45 KB, patch)
2017-08-29 18:45 PDT, Shiyu Zhang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Addy Osmani
Comment 1 2017-08-28 19:26:22 PDT
Thanks for highlighting this as a potential issue. Imo, Speedometer attempting to inject items before app.js is loaded is a bug (however this is the first time I've come across it). My only concern with the proposed solution is whether this pattern for readiness is consistent with how developers use jQuery.
Ryosuke Niwa
Comment 2 2017-08-28 23:45:57 PDT
The fix looks good. Could you create a patch in accordance with https://webkit.org/contributing-code/ ? The easiest way to do this would be to run "./Tools/Scripts/wekbit-patch upload 176017 --commit-queue" at the top level directory, and then edit the change log with a description of your change.
Ryosuke Niwa
Comment 3 2017-08-28 23:54:32 PDT
In the future, please cc me on any of Speedometer related bugs. I don't actively look for bugs so I'd totally miss a bug like this, and nobody else working on Speedometer 2.0 can legitimately review your patch since they're not a WebKit reviewer.
Shiyu Zhang
Comment 4 2017-08-29 18:45:32 PDT
Created attachment 319321 [details] Patch
Shiyu Zhang
Comment 5 2017-08-29 18:55:15 PDT
Thanks for reviewing the bug. I have created the patch, PTAL.
Shiyu Zhang
Comment 6 2017-08-29 21:37:58 PDT
I just noticed that BackboneJS suite may meet the same bug as well. The deleteButtons.length in resources/test.js for BackboneJS suite could be zero sometimes as I observed. This is even worse since unlike jQuery suite, BackboneJS suite will never crash even if deleteButtons.length is zero, which makes this bug difficult to be detected. Shall I file another bug to fix this issue?
Ryosuke Niwa
Comment 7 2017-08-30 15:40:35 PDT
(In reply to Shiyu Zhang from comment #6) > I just noticed that BackboneJS suite may meet the same bug as well. The > deleteButtons.length in resources/test.js for BackboneJS suite could be zero > sometimes as I observed. This is even worse since unlike jQuery suite, > BackboneJS suite will never crash even if deleteButtons.length is zero, > which makes this bug difficult to be detected. Shall I file another bug to > fix this issue? Yes, please file a new bug and make it a blocker to 172339.
Ryosuke Niwa
Comment 8 2017-08-30 15:41:38 PDT
Comment on attachment 319321 [details] Patch In the future, please also set cq flag to ? (to ask to be landed via commit queue).
WebKit Commit Bot
Comment 9 2017-08-30 15:45:30 PDT
Comment on attachment 319321 [details] Patch Clearing flags on attachment: 319321 Committed r221401: <http://trac.webkit.org/changeset/221401>
WebKit Commit Bot
Comment 10 2017-08-30 15:45:31 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2017-08-30 15:46:29 PDT
<rdar://problem/34174394>
Note You need to log in before you can comment on or make changes to this bug.