Bug 176017 - Speedometer 2.0: jQuery test fails occasionally
Summary: Speedometer 2.0: jQuery test fails occasionally
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 172339
  Show dependency treegraph
 
Reported: 2017-08-28 01:49 PDT by Shiyu Zhang
Modified: 2017-08-30 20:16 PDT (History)
8 users (show)

See Also:


Attachments
A possible fix for jQuery test (1.42 KB, patch)
2017-08-28 01:49 PDT, Shiyu Zhang
no flags Details | Formatted Diff | Diff
Patch (2.45 KB, patch)
2017-08-29 18:45 PDT, Shiyu Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 4 Shiyu Zhang 2017-08-29 18:45:32 PDT
Created attachment 319321 [details]
Patch
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.
Comment 11 Radar WebKit Bug Importer 2017-08-30 15:46:29 PDT
<rdar://problem/34174394>