Bug 140059 - http/tests/security/cross-frame-access-put.html is racy
Summary: http/tests/security/cross-frame-access-put.html is racy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-04 15:33 PST by Alexey Proskuryakov
Modified: 2015-01-04 16:17 PST (History)
1 user (show)

See Also:


Attachments
proposed fix (66.23 KB, patch)
2015-01-04 15:36 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
with a fixed ChangeLog (66.22 KB, patch)
2015-01-04 15:43 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-01-04 15:33:58 PST
It failed for me under ASan.
Comment 1 Alexey Proskuryakov 2015-01-04 15:36:14 PST
Created attachment 243942 [details]
proposed fix
Comment 2 WebKit Commit Bot 2015-01-04 15:37:20 PST
Attachment 243942 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexey Proskuryakov 2015-01-04 15:43:11 PST
Created attachment 243944 [details]
with a fixed ChangeLog
Comment 4 Darin Adler 2015-01-04 15:47:21 PST
Comment on attachment 243944 [details]
with a fixed ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=243944&action=review

> LayoutTests/http/tests/security/cross-frame-access-put.html:30
> +    // Run the test in main frame after subframe part finishes.
> +    setTimeout(test, 0);

Does this comment mean that we are relying on the fact that the subframe test runs in a zero-duration timer that has already been scheduled, and that all such timers run in the order they are scheduled? If so, then I think the comment is too brief to be clear on this.

> LayoutTests/http/tests/security/resources/cross-frame-iframe-for-put-test.html:248
> +        // This complicates synchronization with main frame, which needs to wait for this
> +        // code to run.

I find this comment frustrating. It states that running on a timer “complicates” things, but not how we deal with the complexity. It would be better to state clearly what we do to make the test work, and optionally give a reason why other simpler techniques would not work. It’s not important for us to say this is complicated.
Comment 5 Alexey Proskuryakov 2015-01-04 16:17:07 PST
Committed <http://trac.webkit.org/r177880>.

Agreed about the comments, re-worded them before landing.