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.

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

fetch client settings object isn't used in fetch the descendants of and link a module script #9499

Closed
allstarschh opened this issue Jul 7, 2023 · 8 comments · Fixed by #9508 or #9510
Assignees

Comments

@allstarschh
Copy link

https://html.spec.whatwg.org/multipage/webappapis.html#fetch-the-descendants-of-and-link-a-module-script

given a fetch client settings object,  ...

But the algorithm doesn't use this fetch client settings object at all.

The fetch client settings object stores various data structures
https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object

@domfarolino
Copy link
Member

Thanks for filing this. I think this is a result of #8253. Prior to that CL, it looks like that https://whatpr.org/html/8253/085d4d1...75e7bf0/webappapis.html#fetch-the-descendants-of-and-link-a-module-script made use of this by for-looping over all of the modules of a script, and then making module requests for them which required the fetch client settings object. After that PR, it looks like we eagerly call into https://tc39.es/ecma262/#sec-InnerModuleLoading which does the looping for us, and ends up in https://html.spec.whatwg.org/multipage/webappapis.html#hostloadimportedmodule which instead uses the "current settings object" for the request it is about to make.

So now I think we can just remove this vestigial argument.

@domfarolino domfarolino self-assigned this Jul 11, 2023
domenic pushed a commit that referenced this issue Jul 12, 2023
Closes #9499; see the rationale there for why this extra parameter existed in the first place.
@allstarschh
Copy link
Author

The algorithm still has the 'a fetch client settings object' argument,
https://html.spec.whatwg.org/multipage/webappapis.html#fetch-the-descendants-of-and-link-a-module-script

To fetch the descendants of and link a module script module script, given **a fetch client settings object**,

perhaps you missed that?

@domfarolino
Copy link
Member

That's embarrassing! Yep, I uploaded #9510 to finish this off.

@allstarschh
Copy link
Author

I've checked the spec again, I am not sure if removing the 'fetch client settings object' is correct, it looks to me now fetching the top-level module script and fetching the sub-modules are using different settings objects?

please correct me if I am wrong.

So now when doing a worker module script fetching, we've fetched the top-level module worker script, (with the fetch client settings object from the document).

now HostLoadImportedModule(referrer, moduleRequest, loadState, payload) is called, https://html.spec.whatwg.org/multipage/webappapis.html#hostloadimportedmodule

in this case, referrer should be the top-level module worker script(?)
then step 6.2.

step 2. Set settingsObject to referencingScript's settings object.

will overwrite the settingsObject with referencingScript's settings object.

and the referencingScript is a JS module script, should be created from
https://html.spec.whatwg.org/multipage/webappapis.html#creating-a-javascript-module-script

  1. Set script's settings object to settings.

and the creating of a JS module script is called from fetch a single module script
step 12, processResponseConsumeBody step 7

  1. If MIME type is a JavaScript MIME type and moduleType is "javascript", then set module script to the result of creating a JavaScript module script given source text, module map settings object, response's URL, and options.

So the sub-modules are fetched by module map settings object,
whereas the top-level module is fetched by fetch client settings object ?

Trace back to the original caller, the module map settings object is from
https://html.spec.whatwg.org/multipage/workers.html#run-a-worker, step 9

  1. Set up a worker environment settings object with realm execution context, outside settings, and unsafeWorkerCreationTime, and let inside settings be the result.

@domfarolino
Copy link
Member

Yes I think the intention is to fetch a worker's descendant module scripts with the "inner settings object", i.e., module map settings object. In any case, if this is a behavior change, I think it would've been introduced by #8253, but I'm not sure that it is a behavior change.

On one hand, the overwriting of the "current settings object" in HostLoadImportedModule() was already present before that PR: https://whatpr.org/html/8253/085d4d1...75e7bf0/webappapis.html#hostloadimportedmodule:~:text=Set%20settings%20object%20%20settingsObject%20%20to%20referencing%20script%20%20referencingScript%20%20%27s%20settings%20object%20.

On the other hand, the descendant-fetching algorithm did pass in fetch client settings object to the now-gone "internal module script graph fetching procedure", which may have indeed used that to fetch "inner" module scripts. I'm not sure since clawing through the diff is incredibly hard at this point since so much has changed.

Perhaps we can ask @nicolo-ribaudo if he thinks the current spec looks right, but I slightly suspect all is well.

@allstarschh
Copy link
Author

oh, but the 'inner settings object' is got from
https://html.spec.whatwg.org/multipage/workers.html#set-up-a-worker-environment-settings-object
whose policy container will be

worker global scope's policy container.

and worker global scope's policy container will be updated after receiving the top-level module worker script,
https://html.spec.whatwg.org/#run-a-worker
step 14, performFetch, step 3.

  1. Initialize worker global scope's policy container given worker global scope, response, and inside settings.

so does that mean fetching the top-level module worker script is using document's settings, whereas fetching sub-modules is using worker global scope's settings ?

@domfarolino
Copy link
Member

so does that mean fetching the top-level module worker script is using document's settings, whereas fetching sub-modules is using worker global scope's settings ?

Yes I think that's the goal

@domenic
Copy link
Member

domenic commented Jul 13, 2023

It looks like there's been a regression there indeed. Let's continue the conversation in a non-closed issue: #9513

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants