[webidl] Assert prototype immutability

[jugglinmike]

This change is 

[domenic]

We already have extensive tests for this (which cover more cases). Search for setPrototypeOf across all files to see.

[w3c-bots]

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 7836ffe
Ignoring 1 changed files:
No files changed

[w3c-bots]

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 7836ffe
Ignoring 1 changed files:
No files changed

[jugglinmike]

It looks like that method is referenced in the following files:

$ grep setPrototypeOf . -rl
./js/behaviours/SetPrototypeOf-window.html
./html/browsers/history/the-location-interface/allow_prototype_cycle_through_location.sub.html
./html/browsers/origin/cross-origin-objects/cross-origin-objects.html
./streams/writable-streams/properties.js
./common/test-setting-immutable-prototype.js

Maybe a little more instructive is searching for the testSettingImmutablePrototype function as defined in the final result above:

$ grep testSettingImmutablePrototype . -rl
./html/browsers/history/the-location-interface/location-prototype-setting-same-origin-domain.sub.html
./html/browsers/history/the-location-interface/location-prototype-setting-goes-cross-origin-domain.sub.html
./html/browsers/history/the-location-interface/location-prototype-setting-cross-origin-domain.sub.html
./html/browsers/history/the-location-interface/location-prototype-setting-same-origin.html
./html/browsers/history/the-location-interface/location-prototype-setting-cross-origin.sub.html
./html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-same-origin.html
./html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-cross-origin-domain.sub.html
./html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-goes-cross-origin-domain.sub.html
./html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-same-origin-domain.sub.html
./html/browsers/the-windowproxy-exotic-object/windowproxy-prototype-setting-cross-origin.sub.html
./common/test-setting-immutable-prototype.js

...but even here, it seems as though we're missing coverage: ServiceWorkerGlobalScope, DedicatedWorkerGlobalScope, SharedWorkerGlobalScope, and WorkerGlobalScope. There may be others today, and there's always the possibility for more being defined in the future. Part of the motivation for maintaining idlharness.js (as I understand it) is to address these details in a generic fashion so they can be applied consistently without the need for additional maintenance. From that perspective, including the assertion here seems like the best way to get coverage of this behavior.

[domenic]

Sure, that might work, although my impression was idlharness.js tests didn't work in workers (and that's why service worker tests have their own IDL testing framework).

[jugglinmike]

I've been vetting this change with workers/interfaces.worker.js, which the WPT server exposes as
workers/interfaces.worker.html. There may be some non-trivial work to get the same behavior for Service Workers, but that would probably help with gh-4210, which is another improvement I've been wanting to contribute.

[domenic]

OK. Well in general I'd prefer you use the same tests as test-setting-immutable-prototype.js, either through refactoring or duplication, instead of the set in this PR (which does strange things like try/catch and then assert the catch wasn't hit, instead of just letting the exception cause test failures).

[domenic]

Marking as required changes per my last comment

[jugglinmike]

Hi @domenic,

I'm just now getting back to this. Although it's my preference to avoid duplication wherever possible, I'm not sure if re-use is an option here. That's because testSettingImmutablePrototype asserts different conditions for objects from another realm, and I don't think we're able to handle that distinction through idlharness.js. That specific condition seems less common than the general case of named properties objects, so I think it's fair to maintain a separate utility for that. I briefly considered invoking testSettingImmutableProtoype from within idlharness.js. That would require all consumers to include an additional resource, though, and that seems a bit onerous to me.

I've taken your second suggestion and duplicated the relevant assertions. What do you think?

[w3c-bots]

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 89aead7
Ignoring 1 changed files:
No files changed

[w3c-bots]

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 65218bb
Ignoring 1 changed files:
No files changed

[w3c-bots]

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 65218bb
Ignoring 1 changed files:
No files changed

[w3c-bots]

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 65218bb
Ignoring 1 changed files:
No files changed

[domenic]

LGTM with test name changes

[domenic]

This isn't testing named properties objects, just global objects.

[jugglinmike]

The referenced specification text applies only to named properties objects. I think that means the surrounding condition needs to change, not the description. Would you agree?

[domenic]

No... I thought you were referencing https://heycam.github.io/webidl/#platform-object-setprototypeof

[domenic]

Named properties objects only apply to Window, so shouldn't be tested in idlharness.

[jugglinmike]

Got it. Looks like the spec reference needs updating, too

[jugglinmike]

@domenic I've pushed up another commit to address your latest feedback.

[jugglinmike]

@domenic Would you feel comfortable merging this on my behalf? Or would you like me to get another review first?

[domenic]

Oh, I thought you had merge privileges. I'm happy to do so.