Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WebDriver BiDi] Fix issue with memory cache in Chrome #49904

Merged
merged 3 commits into from
Jan 8, 2025
Merged

[WebDriver BiDi] Fix issue with memory cache in Chrome #49904

merged 3 commits into from
Jan 8, 2025
+32 −25

Conversation

Lightning00Blade
Copy link
Contributor

It seem that Chrome sends only 1 request when the there are multiple scripts with the same URL, this look like optimization on our end.

Tested locally with HTML:

<!doctype html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
    <script src="./console.js"></script>
    <script src="./console.js"></script>
  </head>
  <body></body>
</html>

And JS:

console.log(2);

@Lightning00Blade Lightning00Blade changed the title [WebDriver BiDi] Fix issue with cached JS [WebDriver BiDi] Fix issue with cached JS in Chrome Jan 3, 2025
@juliandescottes
Copy link
Contributor

For images I explicitly removed the cache tests because the spec doesn't guarantee that fetch will be used, if the image is already in the document's cached resources.

However for scripts I think the HTML spec guarantees that we should hit fetch for each script tag. Of course it can still then hit the cache and avoid the network layer, but I think we should still see a network event (with fromCache: true) for each tag.

At least that's what I understand when reading https://html.spec.whatwg.org/#script-processing-model:fetch-a-classic-script ?

@OrKoN
Copy link
Contributor

OrKoN commented Jan 7, 2025

The in-memory caches are not yet specified in the HTML spec:

In practice, due to the as-yet-unspecified memory cache (see issue #6110) the resource may only be fetched once in WebKit and Blink-based browsers. Additionally, as long as all module types are mutually exclusive, the module type check in fetch a single module script will fail for at least one of the imports, so at most one module evaluation will occur.

I think from the BiDi spec perspective we have to report the actual browser behavior here. Chrome's implementation I believe is here https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc;l=1449;drc=f39c57f31413abcb41d3068cfb2c7a1718003cc5 @juliandescottes wdyt about landing this test change with a comment about this issue?

@Lightning00Blade Lightning00Blade changed the title [WebDriver BiDi] Fix issue with cached JS in Chrome [WebDriver BiDi] Fix issue with memory cache in Chrome Jan 7, 2025
@juliandescottes
Copy link
Contributor

We already had a similar conversation on #48446 :) (which I guess we can close once this lands)

It's not clear to me if the scope of the memory cache will apply to loading classic scripts on top of modules, but it's true that the current behavior is not clearly specified, or rather even if it's specified, browsers don't really follow the spec. From the investigation on https://bugzilla.mozilla.org/show_bug.cgi?id=1922527#c1 it sounds like for classic scripts we should always end up doing a fetch, but maybe that will change once memory cache is actually specified.

For now sounds fine to update the tests, with comments pointing to the memory cache open issue.

I think from the BiDi spec perspective we have to report the actual browser behavior here.

Thinking more about this, I'm wondering how useful (or easy to understand) it is to NOT have events for resources coming from the memory cache vs the HTTP cache. They are both local caches, so clients might have trouble understanding why some resource loads trigger a network event with fromCache: true and others don't. Should we expose memory cache hits as separate events once this is specified?

Also in general tests relying on network features should really try to disable the cache as much as possible, so maybe the inconsistencies between browsers is not a huge problem here.

@OrKoN
Copy link
Contributor

OrKoN commented Jan 8, 2025

I believe we would not have any events for requests served from the memory cache as there will be no request created (so there is nothing to set the bits on). From the user perspective, the request that is served from the memory cache never happens (and not visible in DevTools as well).

OrKoN
OrKoN requested changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the comments to explain the situation with memory caches linking to whatwg/html#6110

@Lightning00Blade Lightning00Blade enabled auto-merge (squash) January 8, 2025 11:39
@Lightning00Blade Lightning00Blade enabled auto-merge (squash) January 8, 2025 11:45
@Lightning00Blade Lightning00Blade merged commit 339eced into master Jan 8, 2025
17 checks passed
@Lightning00Blade Lightning00Blade deleted the fix-bidi-js-cached-issue branch January 8, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN approved these changes

@sadym-chromium sadym-chromium Awaiting requested review from sadym-chromium

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

@jgraham jgraham Awaiting requested review from jgraham

@juliandescottes juliandescottes Awaiting requested review from juliandescottes

@shs96c shs96c Awaiting requested review from shs96c

@whimboo whimboo Awaiting requested review from whimboo

Assignees

@shs96c shs96c

Labels
webdriver-bidi wg-s_browser-tools-testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Lightning00Blade @juliandescottes @OrKoN @shs96c @wpt-pr-bot @moz-wptsync-bot