Closed Bug 1489301 Opened Last year Closed Last year

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

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).
ni? :peterv and :kmag for thoughts
Flags: needinfo?(peterv)
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
(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.
Attachment #9007059 - Attachment is obsolete: true
ni? bz who wants to take a different approach here
(Nevermind - still on PTO 'til tomorrow)
Flags: needinfo?(peterv)
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...
Blocks: 1500950
This is done by adding BackstagePass the exposure set of Window.
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
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
'Exposed=System' is no longer used in any webidl files, so we can kill it.

Depends on D9400
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
The `Window` interface is now exposed on system globals, so the check is now
invalid.

Depends on D9402
Pushed by nika@thelayzells.com:
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
Blocks: 1501124
Blocks: 1501127
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;
Flags: needinfo?(nika)
Backout by dvarga@mozilla.com:
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
Pushed by nika@thelayzells.com:
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
Flags: needinfo?(nika)
Duplicate of this bug: 1164188
Blocks: 1502048
You need to log in before you can comment on or make changes to this bug.