|Summary:||Speedometer 2.0: jQuery test fails occasionally|
|Product:||WebKit||Reporter:||Shiyu Zhang <shiyu.zhang>|
|Component:||Tools / Tests||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||addyo, buildbot, cdumez, commit-queue, lforschler, mathias, rniwa, webkit-bug-importer|
|Version:||WebKit Nightly Build|
|Bug Depends on:|
Description Shiyu Zhang 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.
Comment 1 Addy Osmani 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.
Comment 2 Ryosuke Niwa 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.
Comment 3 Ryosuke Niwa 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.
Comment 5 Shiyu Zhang 2017-08-29 18:55:15 PDT
Thanks for reviewing the bug. I have created the patch, PTAL.
Comment 6 Shiyu Zhang 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?
Comment 7 Ryosuke Niwa 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.
Comment 8 Ryosuke Niwa 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).
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2017-08-30 15:45:31 PDT
All reviewed patches have been landed. Closing bug.