jsdom / webidl2js

Auto-generate JS class structures for Web IDL specifications
MIT License
79 stars 30 forks source link

Stale cached value returned from the getter of a readonly property #256

Closed cdoublev closed 2 years ago

cdoublev commented 2 years ago

In my implementation of CSSStyleSheet.replaceSync(), I have this:

  get cssRules() {
    if (!this._originClean) {
      throw createError(ACCESS_CROSS_ORIGIN_STYLESHEET_ERROR)
    }
    return this._cssRules // Returns an instance of an implementation class, `CSSRuleList`
  }
  replaceSync(text) {
    // ...
    this._cssRules = parseCSSRuleList(text, this) // Returns an instance of an implementation class, `CSSRuleList`
    // ...
}

After running styleSheet.replaceSync('div { color: green }'), styleSheet.cssRules still returns the old reference to CSSRuleList, where eg. styleSheet.cssRules[0].style.color === 'red'. I suspect that the Proxy for CSSStyleSheet.cssRules returns a cached value because it can not detect that this._cssRules has changed.

Is it a bug or the expected behavior? Does a workaround exists?

Related: https://github.com/w3c/csswg-drafts/issues/6994

EDIT:

I confirm it works as I expect when commenting the following lines in utils.js (generated by webidl2js):

function getSameObject(wrapper, prop, creator) {
  if (!wrapper[sameObjectCaches]) {
    wrapper[sameObjectCaches] = Object.create(null);
  }

  // if (prop in wrapper[sameObjectCaches]) {
  //   return wrapper[sameObjectCaches][prop];
  // }

  wrapper[sameObjectCaches][prop] = creator();
  return wrapper[sameObjectCaches][prop];
}

I'm surprised because this would mean that the value returned by the getter implementation of a read-only property should be static, isn't it?

domenic commented 2 years ago

getSameObject() is only invoked for IDL attributes decorated with [SameObject], which, per IDL, always return the same object. It sounds like you want to not return the same object, so in that case, [SameObject] is not appropriate.

Looking at the spec, it seems like the intent is to always return the same CSSRuleList object, which is "live". So changing this._cssRules is not a correct per-spec implementation, since that replaces the object itself; instead, you should modify the innards of this._cssRules, so that the same object keeps being returned, just with different values.

Does that make sense?

cdoublev commented 2 years ago

Perfectly, thank you. I should have taken a look at its IDL signature. I would have seen [SameObject] immediately. I wonder why Firefox seems to replace the object. Do you think it's worth filing an issue on Bugzilla?

domenic commented 2 years ago

Yeah, definitely. Although if Chrome and/or Safari also replace the object then maybe it's worth filing a spec bug.