CSSStyleSheet.replace() reentrancy via thenable getter causes state corruption
Categories
(Core :: DOM: CSS Object Model, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox150 | --- | fixed |
People
(Reporter: ehdgks0627, Assigned: emilio)
References
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/144.0.0.0 Safari/537.36
Steps to reproduce:
StyleSheet::MaybeResolveReplacePromise() calls mReplacePromise->MaybeResolve(this) followed by mReplacePromise = nullptr. Because SpiderMonkey's promise resolution synchronously accesses Get(resolution, "then") per the ECMAScript specification (Promise Resolve Functions, step 9), a getter defined on CSSStyleSheet.prototype.then can re-enter replace() before the nullptr assignment executes. This permanently corrupts the stylesheet's internal state.
- Create a constructable
CSSStyleSheetand adopt it into a document. - Define a getter on
CSSStyleSheet.prototype.thenthat callssheet.replace()once, then returnsundefined. - Call
sheet.replace()and await the returned Promise.
Minimal reproducer:
<!DOCTYPE html>
<script>
"use strict";
(async () => {
const sheet = new CSSStyleSheet();
document.adoptedStyleSheets = [sheet];
let p2 = null;
let once = false;
Object.defineProperty(CSSStyleSheet.prototype, "then", {
configurable: true,
get() {
if (!once) {
once = true;
p2 = sheet.replace("b { color: blue; }");
}
return undefined;
},
});
await sheet.replace("a { color: red; }");
delete CSSStyleSheet.prototype.then;
// Check if p2 settles
const result = await Promise.race([
p2.then(() => "resolved", e => "rejected: " + e),
new Promise(r => setTimeout(() => r("TIMEOUT"), 3000)),
]);
console.log("p2:", result);
// Check if sheet is still modifiable
try { sheet.replaceSync("c {}"); }
catch (e) { console.log("locked:", e.message); }
})();
</script>
Actual results:
- The second `replace()` Promise **never settles** (orphaned). `console.log` prints `p2: TIMEOUT`.
- The sheet is **permanently locked**: `replace()`, `replaceSync()`, `insertRule()`, `deleteRule()` all throw `NotAllowedError` ("Can only be called on modifiable style sheets").
- The second parse completes and silently replaces the sheet's CSS rules, but with no Promise notification.
- In debug builds, `MOZ_ASSERT(!mReplacePromise)` fires at `StyleSheet.cpp:688`.
Expected results:
- Both `replace()` calls should return independently settling Promises.
- The sheet should remain modifiable after both Promises settle.
- Or, the reentrant `replace()` should be rejected, preserving state consistency.
## Root Cause
`layout/style/StyleSheet.cpp`, lines 612-621:
```cpp
void StyleSheet::MaybeResolveReplacePromise() {
MOZ_ASSERT(!!mReplacePromise == ModificationDisallowed());
if (!mReplacePromise) {
return;
}
SetModificationDisallowed(false); // line 618
mReplacePromise->MaybeResolve(this); // line 619 - JS runs here (thenable check)
mReplacePromise = nullptr; // line 620 - destroys reentrant promise
}
MaybeResolve(this) wraps the C++ StyleSheet as a JS object and passes it through the standard ECMAScript promise resolution, which synchronously calls GetProperty(cx, obj, "then") in ResolvePromiseInternal() (js/src/builtin/Promise.cpp:1442). A user-defined getter on CSSStyleSheet.prototype.then executes in-stack.
The call chain:
MaybeResolve(this)
-> Promise::MaybeSomething() // Promise.h:467
-> ToJSValue(cx, stylesheetPtr, &val)
-> JS::ResolvePromise(cx, p, val)
-> ResolvePromiseInternal() // Promise.cpp:1484
-> GetThenValue() // Promise.cpp:1521
-> GetProperty(cx, obj, "then") // Promise.cpp:1442 -- SYNC getter
During the getter, ModificationDisallowed() is already false (cleared at line 618), so the reentrant replace() passes the check at line 662 and:
- Sets
ModificationDisallowed(true)(line 670) - Overwrites
mReplacePromisewith a new Promise (line 689)
After the getter returns, line 620 executes mReplacePromise = nullptr, destroying the new Promise. The invariant !!mReplacePromise == ModificationDisallowed() is permanently broken:
mReplacePromise == nullptrModificationDisallowed() == true
|
||
Updated•1 month ago
|
|
||
Comment 1•1 month ago
|
||
|
Assignee | |
Comment 2•1 month ago
|
||
This sync then access on promise resolution has bit us in the past, see bug 1923344 for example which had the same root cause.
:arai, do you know if this is per spec? Other browsers don't seem to do this.
|
|
Assignee | |
Comment 3•1 month ago
|
||
This fixes the immediate issue, but it's quite the footgun...
|
||
Updated•1 month ago
|
|
||
Comment 4•1 month ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
This sync then access on promise resolution has bit us in the past, see bug 1923344 for example which had the same root cause.
:arai, do you know if this is per spec? Other browsers don't seem to do this.
In the ECMAScript side, it's per spec.
As long as the "resolve" used there is WebIDL's resolve, it calls the resolving function:
https://drafts.csswg.org/cssom/#dom-cssstylesheet-replace
The replace(text) method must run the following steps:
...
4. *In parallel*, do these steps:
...
5. Resolve _promise_ with _sheet_.
and the step 2.e there accessed the "then" property of the resolution value, which can trigger a getter function.
https://tc39.es/ecma262/#sec-createresolvingfunctions
CreateResolvingFunctions ( promise )
...
2. Let resolveSteps be a new Abstract Closure with parameters
(resolution) that captures promise and alreadyResolved and performs the
following steps when called:
...
e. Let then be Completion(Get(resolution, "then")).
There's a proposal to address these similar issues in https://github.com/tc39/proposal-thenable-curtailment
Then, in the CSS side, I think the problem is that the promise is held by a shared slot StyleSheet::mReplacePromise and easily accessible from other replace invocation.
In terms of the spec, the promise created inside the StyleSheet.prototype.replace is not stored anywhere, but captured by the parallel steps.
If we can store the promise in a storage that's allocated per each replace invocation, this could be solved without worrying about the "then" handling,
but of course that means we need to carry the information over the following call path,
and using the shared slot with a kind of lock mechanism may be simpler:
One possible option would be to merge the SetModificationDisallowed with mReplacePromise field ownership.
Like the following:
void SetModificationDisallowedWith(RefPtr<ReplacePromise>& promise) {
mState |= State::ModificationDisallowed; // with some more complex logic
mReplacePromiseDoNotUseDirectly = promise;
}
ReplacePromise* UnsetModificationDisallowed() {
mState &= ~State::ModificationDisallowed; // with some more complex logic
return mReplacePromiseDoNotUseDirectly.forget();
}
with the following consumers:
void StyleSheet::MaybeResolveReplacePromise() {
...
RefPtr promise = UnsetModificationDisallowed();
promise->MaybeResolve(this);
}
void StyleSheet::MaybeRejectReplacePromise() {
...
RefPtr promise = UnsetModificationDisallowed();
promise->MaybeRejectWithNetworkError(
"@import style sheet load failed");
}
|
|
Assignee | |
Comment 5•1 month ago
|
||
Yeah the main issue is resolving the promise potentially triggering script is something a bit unexpected.
|
|
||
Updated•1 month ago
|
|
|
Assignee | |
Updated•1 month ago
|
|
||
Comment 8•1 month ago
|
||
| bugherder | ||
|
||
Updated•12 days ago
|
Description
•