Bug 130632 – Web Replay: resource unique identifiers should be unique-per-frame, not globally

• Component: Page Loading
• Status: RESOLVED INVALID

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 227531 [details]
the patch

[Build Bot]

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]

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]

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]

*** Bug 129389 has been marked as a duplicate of this bug. ***

[Blaze Burg]

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]

Created attachment 227648 [details]
patch.2

[Build Bot]

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]

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]

(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]

Created attachment 227806 [details]
patch.3

[Blaze Burg]

Committed r166264: <http://trac.webkit.org/changeset/166264>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 130785

[Blaze Burg]

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]

Created attachment 227981 [details]
Patch

[Blaze Burg]

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).