1489301 - Consider allowing Window-exposed interfaces to be used in System-exposed interfaces

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently, many WebIDL types are not exposed on BackstagePass. I believe this was done in order to avoid the overhead of creating interface objects for these classes when they aren't needed. For example, the FrameLoader ChromeOnly WebIDL type is only exposed on Window globals, and not on System globals.

Some things, such as the ChromeUtils namespace, are exposed on System globals, as well as Window globals. Due to the WebIDL rules around exposure, this means that every type exposed through ChromeUtils must be exposed on both the System and Window globals. For example, BrowsingContext is exposed on both globals as it is returned from a ChromeUtils method.

I'm trying to add a new object which will not be interacted with frequently through the interface object, but rather as a property or return value from other objects. I want this interface to be returned from an attribute on the BrowsingContext interface. Unfortunately, my interface exposes a FrameLoader property, which in turn exposes a large # of other interfaces only exposed on Window.

It seems like one of these three options could be taken:

 1. Allow interfaces exposed on Window to be used in interfaces exposed on Chrome, as Chrome code can acquire access to objects from Window fairly easily. This would let us keep BackstagePass globals smaller.

 2. Change the default Exposed= value for interfaces to be (Window, Chrome) rather than just Window. This might be OK due to BackstagePass globals having recently been unified.

 3. Perform the infectious exposing and expose everything manually on Chrome which is transitively used by FrameLoader manually. (This may get even worse if we begin exposing properties like WindowProxy or Window from BrowsingContext).

Comment 1

[Nika Layzell [:nika] (ni? for response)]

ni? :peterv and :kmag for thoughts

Comment 2

[Kris Maglione [:kmag]]

Boris ran into this issue in bug 1164188.

I'd lean towards option 1 above, since it's the least invasive option, and pretty much the behavior we have now for things imported via importGlobalProperties.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Allow Window-exposed interfaces to be used in BackstagePass-exposed interfaces

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Kris Maglione [:kmag] from comment #2)
> Boris ran into this issue in bug 1164188.
> 
> I'd lean towards option 1 above, since it's the least invasive option, and
> pretty much the behavior we have now for things imported via
> importGlobalProperties.

Yeah, that's what I was thinking too. Already whipped together a quick patch which (I think) should do the trick.

Sending it to peter for review.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

ni? bz who wants to take a different approach here

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(Nevermind - still on PTO 'til tomorrow)

Comment 7

[Boris Zbarsky [:bzbarsky]]

So my proposal for a different approach was to just get rid of "Exposed=System" altogether and make scopes that currently use that exposure set just expose everything that's exposed on Window.  We could reuse the hashtable table we use for the Window resolve hook and eliminate the extra codegen for the system resolve/enumerate hooks....

This makes it a bit harder to expose things in System but not Window, but we could do it via a Func that checks for system principal and then screens out Window globals...

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Part 1: Expose Window interfaces on System by default,

This is done by adding BackstagePass the exposure set of Window.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Part 2: Remove unnecessary [Exposed=System] annotations,

We don't need to expose on both Window and System anymore, as Window now
implies System. I don't remove unnecessary [Exposed=Window] annotations, as
WebIDL upstream has removed PrimaryGlobal.

Depends on D9398

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Part 3: Expose all System-only objects on Window,

There is a limited number of these, and this allows me to completely remove
mention of the 'System' global.

In the future System-only exposure could be achieved using a [Func] enabler.

Depends on D9399

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Part 4: Remove references to 'System' from WebIDL.py,

'Exposed=System' is no longer used in any webidl files, so we can kill it.

Depends on D9400

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Part 5: Fix broken [Func] condition assuming Window object,

This condition unwraps the global to a window, and doesn't check before
dereferencing. This is no longer valid now that the corresponding interfaces
are exposed on BackstagePass.

Depends on D9401

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1489301 - Part 6: Fix test expecting interfaces not exposed on System,

The `Window` interface is now exposed on system globals, so the check is now
invalid.

Depends on D9402

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8fb091198e
Part 1: Expose Window interfaces on System by default, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ca10fa3772
Part 2: Fix broken [Func] condition assuming Window object, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/168cf9cea716
Part 3: Fix test expecting interfaces not exposed on System, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/f34bc8a40bec
Part 4: Remove unnecessary [Exposed=System] annotations, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/529524df76a6
Part 5: Expose all System-only objects on Window, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0823540b44
Part 6: Remove references to 'System' from WebIDL.py, r=bzbarsky

Comment 15

[Daniel Varga [:dvarga]]

Backed out 6 changesets (Bug 1489301) for Linting opt failure at /builds/worker/checkouts/gecko/dom/bindings/parser/WebIDL.py

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=1c0823540b44ff83a6319a363aab6e017faddaf4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=207146932&repo=mozilla-inbound&lineNumber=1904

[task 2018-10-23T00:34:20.450Z] TEST-PASS | ShoulTraceback (most recent call last):
[task 2018-10-23T00:34:20.451Z]   File "/builds/worker/checkouts/gecko/dom/bindings/parser/runtests.py", line 70, in run_tests
[task 2018-10-23T00:34:20.451Z]     _test.WebIDLTest.__call__(WebIDL.Parser(), harness)
[task 2018-10-23T00:34:20.451Z]   File "/builds/worker/checkouts/gecko/dom/bindings/parser/tests/test_extended_attributes.py", line 21, in WebIDLTest
[task 2018-10-23T00:34:20.452Z]     results = parser.finish()
[task 2018-10-23T00:34:20.452Z]   File "/builds/worker/checkouts/gecko/dom/bindings/parser/WebIDL.py", line 7020, in finish
[task 2018-10-23T00:34:20.452Z]     production.validate()
[task 2018-10-23T00:34:20.453Z]   File "/builds/worker/checkouts/gecko/dom/bindings/parser/WebIDL.py", line 1239, in validate
[task 2018-10-23T00:34:20.453Z]     member.validate()
[task 2018-10-23T00:34:20.453Z]   File "/builds/worker/checkouts/gecko/dom/bindings/parser/WebIDL.py", line 4170, in validate
[task 2018-10-23T00:34:20.453Z]     IDLInterfaceMember.validate(self)
[task 2018-10-23T00:34:20.453Z]   File "/builds/worker/checkouts/gecko/dom/bindings/parser/WebIDL.py", line 3582, in validate
[task 2018-10-23T00:34:20.454Z]     [self.location])
[task 2018-10-23T00:34:20.454Z] WebIDLError: error: [Pref] used on an interface member that is not main-thread-only, <unknown> line 4:27
[task 2018-10-23T00:34:20.454Z]           [Pref="foo.bar"] attribute byte b;

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f4c84bebf0
Backed out 6 changesets for Linting opt failure at /builds/worker/checkouts/gecko/dom/bindings/parser/WebIDL.py

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22888a1208ab
Part 1: Expose Window interfaces on System by default, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5a91eadc49
Part 2: Fix broken [Func] condition assuming Window object, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/a43d864f8502
Part 3: Fix test expecting interfaces not exposed on System, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfcfc27ab30
Part 4: Remove unnecessary [Exposed=System] annotations, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ac98041af7
Part 5: Expose all System-only objects on Window, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/59160a8260a0
Part 6: Remove references to 'System' from WebIDL.py, r=bzbarsky

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/22888a1208ab
https://hg.mozilla.org/mozilla-central/rev/dc5a91eadc49
https://hg.mozilla.org/mozilla-central/rev/a43d864f8502
https://hg.mozilla.org/mozilla-central/rev/fbfcfc27ab30
https://hg.mozilla.org/mozilla-central/rev/d8ac98041af7
https://hg.mozilla.org/mozilla-central/rev/59160a8260a0