Bug 130632 - Web Replay: resource unique identifiers should be unique-per-frame, not globally
Summary: Web Replay: resource unique identifiers should be unique-per-frame, not globally
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
: 129389 (view as bug list)
Depends on: 130785
Blocks: 129391
  Show dependency treegraph
 
Reported: 2014-03-21 20:31 PDT by BJ Burg
Modified: 2014-03-27 14:43 PDT (History)
11 users (show)

See Also:


Attachments
the patch (3.97 KB, patch)
2014-03-21 20:53 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
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 Details
patch.2 (10.42 KB, patch)
2014-03-24 08:47 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
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 Details
patch.3 (10.54 KB, patch)
2014-03-25 15:18 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch (2.49 KB, patch)
2014-03-27 14:35 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 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.
Comment 1 BJ Burg 2014-03-21 20:53:13 PDT
Created attachment 227531 [details]
the patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 BJ Burg 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).
Comment 5 BJ Burg 2014-03-23 08:28:02 PDT
*** Bug 129389 has been marked as a duplicate of this bug. ***
Comment 6 BJ Burg 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.
Comment 7 BJ Burg 2014-03-24 08:47:11 PDT
Created attachment 227648 [details]
patch.2
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 BJ Burg 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 :|
Comment 11 BJ Burg 2014-03-25 15:18:06 PDT
Created attachment 227806 [details]
patch.3
Comment 12 BJ Burg 2014-03-25 15:57:22 PDT
Committed r166264: <http://trac.webkit.org/changeset/166264>
Comment 13 WebKit Commit Bot 2014-03-26 11:28:05 PDT
Re-opened since this is blocked by bug 130785
Comment 14 BJ Burg 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.
Comment 15 BJ Burg 2014-03-27 14:35:24 PDT
Created attachment 227981 [details]
Patch
Comment 16 BJ Burg 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).