RESOLVED INVALID 130632
Web Replay: resource unique identifiers should be unique-per-frame, not globally
https://bugs.webkit.org/show_bug.cgi?id=130632
Summary Web Replay: resource unique identifiers should be unique-per-frame, not globally
Blaze Burg
Reported 2014-03-21 20:31:04 PDT
For replay purposes, we want to deterministically assign resource load identifiers to resource loaders, provided that the resource loaders are created in the same order. However, the current implementation uses a static variable so the identifiers are unique per web process.
Attachments
the patch (3.97 KB, patch)
2014-03-21 20:53 PDT, Blaze Burg
no flags
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (525.08 KB, application/zip)
2014-03-21 22:53 PDT, Build Bot
no flags
patch.2 (10.42 KB, patch)
2014-03-24 08:47 PDT, Blaze Burg
no flags
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (540.31 KB, application/zip)
2014-03-24 09:48 PDT, Build Bot
no flags
patch.3 (10.54 KB, patch)
2014-03-25 15:18 PDT, Blaze Burg
no flags
Patch (2.49 KB, patch)
2014-03-27 14:35 PDT, Blaze Burg
no flags
Blaze Burg
Comment 1 2014-03-21 20:53:13 PDT
Created attachment 227531 [details] the patch
Build Bot
Comment 2 2014-03-21 22:53:08 PDT
Comment on attachment 227531 [details] the patch Attachment 227531 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6489168813228032 New failing tests: http/tests/cache/iframe-304-crash.html security/block-test.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html http/tests/loading/307-after-303-after-post.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html loader/go-back-cached-main-resource.html
Build Bot
Comment 3 2014-03-21 22:53:11 PDT
Created attachment 227534 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Blaze Burg
Comment 4 2014-03-22 10:07:30 PDT
I will look at the test failures later today, but let me know if something obvious jumps out (asides from being WK2-only).
Blaze Burg
Comment 5 2014-03-23 08:28:02 PDT
*** Bug 129389 has been marked as a duplicate of this bug. ***
Blaze Burg
Comment 6 2014-03-23 11:53:01 PDT
One issue: the assignedUrlsCache in InjectedBundlePage maps resource identifiers to URLs, but doesn't disambiguate based on the associated frame. So, some of the dumped resource callbacks have the wrong URL. I think I can fix this by making the map key include the frame id and the resource id.
Blaze Burg
Comment 7 2014-03-24 08:47:11 PDT
Build Bot
Comment 8 2014-03-24 09:48:01 PDT
Comment on attachment 227648 [details] patch.2 Attachment 227648 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5452224658407424 New failing tests: media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html security/block-test.html loader/go-back-cached-main-resource.html http/tests/loading/307-after-303-after-post.html
Build Bot
Comment 9 2014-03-24 09:48:04 PDT
Created attachment 227662 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Blaze Burg
Comment 10 2014-03-25 14:28:55 PDT
(In reply to comment #8) > (From update of attachment 227648 [details]) > Attachment 227648 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/5452224658407424 > > New failing tests: > security/block-test.html > loader/go-back-cached-main-resource.html > http/tests/loading/307-after-303-after-post.html The remaining failures are caused because ProgressTracker::reset() is called at DOMContentLoaded or thereabouts, and we were resetting the unique identifier inside reset(). So the unique identifiers were not unique :|
Blaze Burg
Comment 11 2014-03-25 15:18:06 PDT
Blaze Burg
Comment 12 2014-03-25 15:57:22 PDT
WebKit Commit Bot
Comment 13 2014-03-26 11:28:05 PDT
Re-opened since this is blocked by bug 130785
Blaze Burg
Comment 14 2014-03-26 11:33:06 PDT
Sigh... looks like NetworkProcess needs the same modification as WKTR. Working hypothesis: NetworkProcess currently deals with identifiers and no frames, so it is probably mistakenly cancelling main resource loads (which always have the same identifier) early, thus the onload events don't fire somehow, and the tests time out.
Blaze Burg
Comment 15 2014-03-27 14:35:24 PDT
Blaze Burg
Comment 16 2014-03-27 14:43:55 PDT
Going to close this bug as invalid, as the approach suggested has too many downsides. Much of the design of NetworkProcess assumes resource loader identifiers are globally unique. I think an alternate approach based on identifier logging should be simpler and have basically no impact on other uses of identifiers. The latest patch uploaded here demonstrates the basic logging functionality needed. It may be moved to a different class like document loader. The patch will be rolled into the main network replay patch (wkbug.com/129391).
Note You need to log in before you can comment on or make changes to this bug.