[CSS Modules] Fix CHECK when styles are preloaded from the cache [chromium/src : main]
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
ScriptForbiddenScope::AllowUserAgentScript allow_script;Kurt Catti-SchmidtCan you limit this scope to the call that actually needs it?
It'd be great to have a better explanation as to what UA script is running and hence why this is needed
Kurt Catti-SchmidtThis is the CHECK:
[43400:34332:0203/163108.125:FATAL:third_party\blink\renderer\platform\bindings\v8_per_isolate_data.cc:72] Check failed: !ScriptForbiddenScope::IsScriptForbidden().
(a988.861c): Break instruction exception - code 80000003 (first chance)It's due to the `ToV8` call in `CreateCSSWrapperSyntheticModuleScript`, which the other module types don't need. Previously, this was only possible to call in script, but with Declarative CSS Modules and now modulepreload, it can be called via parsing. Declarative CSS Modules already has a `AllowUserAgentScript`, see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_element.cc;bpv=1;bpt=1;l=262?q=style_element.cc&ss=chromium&gsn=AddImportMapEntry&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dthird_party%2Fblink%2Frenderer%2Fcore%2Fcss%2Fstyle_element.cc%23U6hWJWctuQ7IiaMYhUrf5vW-PSalk6hfS87-e1FP9FQ
I could move the `ScriptForbiddenScope` to `CreateCSSWrapperSyntheticModuleScript`, but it seems like this is the correct place to have it - we're starting to execute script on behalf of the parser to add the preloaded module in the module map, so we should allow script for the rest of the scope.
Here's the stack:
```
00 base!base::ImmediateCrash
01 base!logging::LogMessage::HandleFatal
02 base!logging::LogMessage::Flush::<lambda_1>::operator()
03 base!absl::cleanup_internal::Storage<`lambda at ..\..\base\logging.cc:739:40'>::InvokeCallback
04 base!absl::Cleanup<absl::cleanup_internal::Tag,`lambda at ..\..\base\logging.cc:739:40'>::~Cleanup
05 base!logging::LogMessage::Flush
06 base!logging::LogMessage::~LogMessage
07 base!logging::`anonymous namespace'::CheckLogMessage::~CheckLogMessage
08 base!logging::`anonymous namespace'::CheckLogMessage::~CheckLogMessage
09 base!std::__Cr::default_delete<logging::LogMessage>::operator()
0a base!std::__Cr::unique_ptr<logging::LogMessage,std::__Cr::default_delete<logging::LogMessage> >::reset
0b base!logging::CheckNoreturnError::~CheckNoreturnError
0c blink_platform!blink::BeforeCallEnteredCallback
0d v8!v8::CallDepthScope<1>::CallDepthScope<1>
0e v8!v8::EnterV8InternalScope<v8::InternalEscapableScope,1>::EnterV8InternalScope<v8::InternalEscapableScope,1>
0f v8!v8::Function::NewInstanceWithSideEffectType
10 blink_platform!blink::V8ObjectConstructor::NewInstance
11 blink_platform!blink::V8PerContextData::CreateWrapperFromCacheSlowCase
12 blink_platform!blink::V8PerContextData::CreateWrapperFromCache
13 blink_platform!blink::V8DOMWrapper::CreateWrapper
14 blink_platform!blink::ScriptWrappable::Wrap
15 blink_platform!blink::ScriptWrappable::ToV8
16 blink_core!blink::ToV8Traits<blink::CSSStyleSheet>::ToV8
17 blink_core!blink::ValueWrapperSyntheticModuleScript::CreateCSSWrapperSyntheticModuleScript
18 blink_core!blink::ModuleScriptLoader::NotifyFetchFinishedSuccess
19 blink_core!blink::DocumentModuleScriptFetcher::NotifyFinished
1a blink_platform!blink::Resource::DidAddClient
1b blink_platform!blink::Resource::AddClient
1c blink_platform!blink::ResourceClient::SetResource
1d blink_platform!blink::ResourceFetcher::RequestResource
1e blink_core!blink::ScriptResource::Fetch
1f blink_core!blink::DocumentModuleScriptFetcher::Fetch
20 blink_core!blink::ModuleScriptLoader::FetchInternal
21 blink_core!blink::ModuleScriptLoader::Fetch
22 blink_core!blink::ModuleMap::FetchSingleModuleScript
23 blink_core!blink::ModulatorImplBase::FetchSingle
24 blink_core!blink::PreloadHelper::ModulePreloadIfNeeded
25 blink_core!blink::LinkLoader::LoadLink
26 blink_core!blink::HTMLLinkElement::LoadLink
27 blink_core!blink::LinkStyle::Process
28 blink_core!blink::HTMLLinkElement::Process
29 blink_core!blink::HTMLLinkElement::InsertedInto
2a blink_core!blink::ContainerNode::NotifyNodeInsertedInternal
2b blink_core!blink::ContainerNode::NotifyNodeInserted
2c blink_core!blink::ContainerNode::ParserAppendChild
2d blink_core!blink::Insert
2e blink_core!blink::ExecuteInsertTask
2f blink_core!blink::HTMLConstructionSite::ExecuteTask
30 blink_core!blink::HTMLConstructionSite::ExecuteQueuedTasks
31 blink_core!blink::HTMLTreeBuilder::ConstructTree
32 blink_core!blink::HTMLDocumentParser::ConstructTreeFromToken
33 blink_core!blink::HTMLDocumentParser::PumpTokenizer
34 blink_core!blink::HTMLDocumentParser::PumpTokenizerIfPossible
35 blink_core!blink::HTMLDocumentParser::DeferredPumpTokenizerIfPossible
```
Friendly ping on this @yoav....@shopify.com. Do you agree with my comment above, or should we go with something more scoped, like https://chromium-review.googlesource.com/c/chromium/src/+/7536427/1..2?
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
@hiro...@chromium.org - Yoav seems to be out-of-office, can you please take a look at this change?
Related details
- Hiroshige Hayashizaki
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Shouldn't the scope be just around FetchSingle then?
Related details
- Hiroshige Hayashizaki
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
ScriptForbiddenScope::AllowUserAgentScript allow_script;- Hiroshige Hayashizaki
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
ScriptForbiddenScope::AllowUserAgentScript allow_script;Where is the `ScriptForbiddenScope` is placed in the stack trace?
Related details
- Kurt Catti-Schmidt
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
It would be between these two lines:
```
23 blink_core!blink::ModulatorImplBase::FetchSingle
24 blink_core!blink::PreloadHelper::ModulePreloadIfNeeded
- Hiroshige Hayashizaki
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
Er, I meant where is the existing `ScriptForbiddenScope` that forbids the script execution here and thus causing the crash?
Related details
- Kurt Catti-Schmidt
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
Oh yeah, duh. It's in `ContainerNode::NotifyNodeInsertedInternal`:
Basically, the parser doesn't expect script to execute while it's parsing (which makes sense), but creating a stylesheet module via `modulepreload` can't modify the DOM so it's safe.
Related details
- Hiroshige Hayashizaki
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
I removed this `ScriptForbiddenScope` and the `ScriptForbiddenScope::AllowUserAgentScript` is no longer necessary, confirming that this is the only `ScriptForbiddenScope` in the stack.
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
ScriptForbiddenScope::AllowUserAgentScript allow_script;Can you add a '{' above this line and a '}' below FetchSingle, to ensure the scope is limited?
Related details
- Hiroshige Hayashizaki
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
ScriptForbiddenScope::AllowUserAgentScript allow_script;Can you add a '{' above this line and a '}' below FetchSingle, to ensure the scope is limited?
- Hiroshige Hayashizaki
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
Thanks for clarification!
Basically, the parser doesn't expect script to execute while it's parsing (which makes sense), but creating a stylesheet module via modulepreload can't modify the DOM so it's safe.
Then, I feel the `ScriptForbiddenScope::AllowUserAgentScript` should be placed around `CreateCSSWrapperSyntheticModuleScript` (or even only around its `ToV8()` call), because we still don't want to execute general scripts inside `FetchSingle()` and `NotifyNodeInsertedInternal()`.
`FetchSingle()` could synchronously create modules (and hitting the `ToV8()` call as a part of https://html.spec.whatwg.org/#creating-a-css-module-script), but still shouldn't execute scripts (https://html.spec.whatwg.org/#run-a-module-script).
So the script execution during creating a module (i.e. `CreateCSSWrapperSyntheticModuleScript`) should be the only exception here.
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Commit-Queue | +1 |
1 comment
ScriptForbiddenScope::AllowUserAgentScript allow_script;Awesome, thank you for looking into this! This is addressed in the latest version.
Related details
- Hiroshige Hayashizaki
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted Code-Review+1![]( "Open in Gerrit")
- Hiroshige Hayashizaki
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57887.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Hiroshige Hayashizaki
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Hiroshige Hayashizaki
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[CSS Modules] Fix CHECK when styles are preloaded from the cache
When style modules are preloaded from the cache, there's a
ScriptForbiddenScope deep in the stack that gets hit. This change fixes
the CHECK by allowing user agent scripts for this scenario.
A test was added that hits the DCHECK without this fix.
- M third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc
- A third_party/blink/web_tests/external/wpt/preload/modulepreload-multiple.html
Code-Review: +1 by Yoav Weiss (@Shopify)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57887
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |