performing cross-context instanceof checks

Hi,

For a long time Firefox's behaviour for instanceof checks on DOM objects, when the right-hand side interface object comes from a different window from the object on the left, has differed from other browsers.  For example,

  otherWindow.document instanceof Node

evaluates to true in Firefox and false in other browsers.

Recently, Web IDL has been updated to return false for cross-context instanceof checks like this.  Fixing this (bug 1360715) is not a small project, since there are many places in chrome code where we rely on instanceof working cross-context.  It is a useful check to be able to do, so at some point we'll likely extend Web IDL with a way to do this, but for now there is no standardised way to do so.

For use in the meantime, I just landed bug 1428531 on inbound, which adds a new chrome-only static method "isInstance" to Web IDL defined interfaces, so you can write for example:

  Document.isInstance(otherWindow.document)

So that we don't have even more call sites we need to fix up once we start looking at bug 1360715, please try to use isInstance rather than instanceof if you need to do cross-context DOM object tests from chrome JS.

Thanks,

Cameron
0
Cameron
1/11/2018 5:29:32 AM
mozilla.dev.platform 6145 articles. 0 followers. Post Follow

7 Replies
13 Views

Similar Articles

[PageSpeed] 29

On 11/01/2018 05:29, Cameron McCormack wrote:
> For use in the meantime, I just landed bug 1428531 on inbound, which adds a new chrome-only static method "isInstance" to Web IDL defined interfaces, so you can write for example:
> 
>    Document.isInstance(otherWindow.document)
> 
> So that we don't have even more call sites we need to fix up once we start looking at bug 1360715, please try to use isInstance rather than instanceof if you need to do cross-context DOM object tests from chrome JS.

Assuming we're happy to update all sites (not only sites where we're 
sure the DOM instance is sometimes cross-context), this type of thing is 
relatively easy to automatically rewrite with an xpcshell script like 
the ones Florian has been using to update other conventions (e.g. use of 
Services.prefs instead of manually Cc[...].getService(Ci.nsIPrefBranch) 
etc. ). If we rewrite the extant callsites, we can add eslint checks 
that warn and can do similar rewrites for people, which will be better 
at avoiding new callsites than just convention (which people are liable 
to forget in a few days/weeks/months), and would make the transition 
relatively "easy".

~ Gijs
0
Gijs
1/11/2018 2:58:23 PM
This could be an issue for WebExtensions as well. I think the contentscript
sandbox runs in a different compartment.

On Thu, Jan 11, 2018 at 3:58 PM, Gijs Kruitbosch <gijskruitbosch@gmail.com>
wrote:

> On 11/01/2018 05:29, Cameron McCormack wrote:
>
>> For use in the meantime, I just landed bug 1428531 on inbound, which adds
>> a new chrome-only static method "isInstance" to Web IDL defined interfaces,
>> so you can write for example:
>>
>>    Document.isInstance(otherWindow.document)
>>
>> So that we don't have even more call sites we need to fix up once we
>> start looking at bug 1360715, please try to use isInstance rather than
>> instanceof if you need to do cross-context DOM object tests from chrome JS.
>>
>
> Assuming we're happy to update all sites (not only sites where we're sure
> the DOM instance is sometimes cross-context), this type of thing is
> relatively easy to automatically rewrite with an xpcshell script like the
> ones Florian has been using to update other conventions (e.g. use of
> Services.prefs instead of manually Cc[...].getService(Ci.nsIPrefBranch)
> etc. ). If we rewrite the extant callsites, we can add eslint checks that
> warn and can do similar rewrites for people, which will be better at
> avoiding new callsites than just convention (which people are liable to
> forget in a few days/weeks/months), and would make the transition
> relatively "easy".
>
> ~ Gijs
>
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
0
Tom
1/11/2018 4:12:37 PM
Based on what Cameron wrote, other browsers already return false if 
things get mixed, so hopefully the WebExtensions side of the problem is 
still limited?

~ Gijs

On 11/01/2018 16:12, Tom Schuster wrote:
> This could be an issue for WebExtensions as well. I think the contentscript
> sandbox runs in a different compartment.
> 
> On Thu, Jan 11, 2018 at 3:58 PM, Gijs Kruitbosch <gijskruitbosch@gmail.com>
> wrote:
> 
>> On 11/01/2018 05:29, Cameron McCormack wrote:
>>
>>> For use in the meantime, I just landed bug 1428531 on inbound, which adds
>>> a new chrome-only static method "isInstance" to Web IDL defined interfaces,
>>> so you can write for example:
>>>
>>>     Document.isInstance(otherWindow.document)
>>>
>>> So that we don't have even more call sites we need to fix up once we
>>> start looking at bug 1360715, please try to use isInstance rather than
>>> instanceof if you need to do cross-context DOM object tests from chrome JS.
>>>
>>
>> Assuming we're happy to update all sites (not only sites where we're sure
>> the DOM instance is sometimes cross-context), this type of thing is
>> relatively easy to automatically rewrite with an xpcshell script like the
>> ones Florian has been using to update other conventions (e.g. use of
>> Services.prefs instead of manually Cc[...].getService(Ci.nsIPrefBranch)
>> etc. ). If we rewrite the extant callsites, we can add eslint checks that
>> warn and can do similar rewrites for people, which will be better at
>> avoiding new callsites than just convention (which people are liable to
>> forget in a few days/weeks/months), and would make the transition
>> relatively "easy".
>>
>> ~ Gijs
>>
>> _______________________________________________
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>

0
Gijs
1/11/2018 4:16:17 PM
IIRC Blink uses a different mechanism (called "separate worlds") to allow
extensions to interact with content, whereas we use a separate global +
xrays. So this likely will be a problem for WebExtensions, and we'll
presumably need a sandboxOption to opt into the old behavior.

On Thu, Jan 11, 2018 at 8:16 AM, Gijs Kruitbosch <gijskruitbosch@gmail.com>
wrote:

> Based on what Cameron wrote, other browsers already return false if things
> get mixed, so hopefully the WebExtensions side of the problem is still
> limited?
>
> ~ Gijs
>
> On 11/01/2018 16:12, Tom Schuster wrote:
>
>> This could be an issue for WebExtensions as well. I think the
>> contentscript
>> sandbox runs in a different compartment.
>>
>> On Thu, Jan 11, 2018 at 3:58 PM, Gijs Kruitbosch <
>> gijskruitbosch@gmail.com>
>> wrote:
>>
>> On 11/01/2018 05:29, Cameron McCormack wrote:
>>>
>>> For use in the meantime, I just landed bug 1428531 on inbound, which adds
>>>> a new chrome-only static method "isInstance" to Web IDL defined
>>>> interfaces,
>>>> so you can write for example:
>>>>
>>>>     Document.isInstance(otherWindow.document)
>>>>
>>>> So that we don't have even more call sites we need to fix up once we
>>>> start looking at bug 1360715, please try to use isInstance rather than
>>>> instanceof if you need to do cross-context DOM object tests from chrome
>>>> JS.
>>>>
>>>>
>>> Assuming we're happy to update all sites (not only sites where we're sure
>>> the DOM instance is sometimes cross-context), this type of thing is
>>> relatively easy to automatically rewrite with an xpcshell script like the
>>> ones Florian has been using to update other conventions (e.g. use of
>>> Services.prefs instead of manually Cc[...].getService(Ci.nsIPrefBranch)
>>> etc. ). If we rewrite the extant callsites, we can add eslint checks that
>>> warn and can do similar rewrites for people, which will be better at
>>> avoiding new callsites than just convention (which people are liable to
>>> forget in a few days/weeks/months), and would make the transition
>>> relatively "easy".
>>>
>>> ~ Gijs
>>>
>>> _______________________________________________
>>> dev-platform mailing list
>>> dev-platform@lists.mozilla.org
>>> https://lists.mozilla.org/listinfo/dev-platform
>>>
>>>
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
0
Bobby
1/11/2018 5:07:10 PM
On Thu, Jan 11, 2018 at 05:12:37PM +0100, Tom Schuster wrote:
>This could be an issue for WebExtensions as well. I think the contentscript
>sandbox runs in a different compartment.

It runs in a different compartment, but the DOM constructors it 
has access to come from the same content window as the nodes it 
would be checking, so this isn't an issue.

>On Thu, Jan 11, 2018 at 3:58 PM, Gijs Kruitbosch <gijskruitbosch@gmail.com>
>wrote:
>
>> On 11/01/2018 05:29, Cameron McCormack wrote:
>>
>>> For use in the meantime, I just landed bug 1428531 on inbound, which adds
>>> a new chrome-only static method "isInstance" to Web IDL defined interfaces,
>>> so you can write for example:
>>>
>>>    Document.isInstance(otherWindow.document)
>>>
>>> So that we don't have even more call sites we need to fix up once we
>>> start looking at bug 1360715, please try to use isInstance rather than
>>> instanceof if you need to do cross-context DOM object tests from chrome JS.
>>>
>>
>> Assuming we're happy to update all sites (not only sites where we're sure
>> the DOM instance is sometimes cross-context), this type of thing is
>> relatively easy to automatically rewrite with an xpcshell script like the
>> ones Florian has been using to update other conventions (e.g. use of
>> Services.prefs instead of manually Cc[...].getService(Ci.nsIPrefBranch)
>> etc. ). If we rewrite the extant callsites, we can add eslint checks that
>> warn and can do similar rewrites for people, which will be better at
>> avoiding new callsites than just convention (which people are liable to
>> forget in a few days/weeks/months), and would make the transition
>> relatively "easy".
>>
>> ~ Gijs
0
Kris
1/11/2018 8:06:09 PM
On Thu, Jan 11, 2018 at 12:06 PM, Kris Maglione <kmaglione@mozilla.com>
wrote:

> On Thu, Jan 11, 2018 at 05:12:37PM +0100, Tom Schuster wrote:
>
>> This could be an issue for WebExtensions as well. I think the
>> contentscript
>> sandbox runs in a different compartment.
>>
>
> It runs in a different compartment, but the DOM constructors it has access
> to come from the same content window as the nodes it would be checking, so
> this isn't an issue.


Oh, I guess Xrays behavior would be unaffected by Cameron's proposal.
Ignore my previous message.


>
> On Thu, Jan 11, 2018 at 3:58 PM, Gijs Kruitbosch <gijskruitbosch@gmail.com
>> >
>> wrote:
>>
>> On 11/01/2018 05:29, Cameron McCormack wrote:
>>>
>>> For use in the meantime, I just landed bug 1428531 on inbound, which adds
>>>> a new chrome-only static method "isInstance" to Web IDL defined
>>>> interfaces,
>>>> so you can write for example:
>>>>
>>>>    Document.isInstance(otherWindow.document)
>>>>
>>>> So that we don't have even more call sites we need to fix up once we
>>>> start looking at bug 1360715, please try to use isInstance rather than
>>>> instanceof if you need to do cross-context DOM object tests from chrome
>>>> JS.
>>>>
>>>>
>>> Assuming we're happy to update all sites (not only sites where we're sure
>>> the DOM instance is sometimes cross-context), this type of thing is
>>> relatively easy to automatically rewrite with an xpcshell script like the
>>> ones Florian has been using to update other conventions (e.g. use of
>>> Services.prefs instead of manually Cc[...].getService(Ci.nsIPrefBranch)
>>> etc. ). If we rewrite the extant callsites, we can add eslint checks that
>>> warn and can do similar rewrites for people, which will be better at
>>> avoiding new callsites than just convention (which people are liable to
>>> forget in a few days/weeks/months), and would make the transition
>>> relatively "easy".
>>>
>>> ~ Gijs
>>>
>> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
0
Bobby
1/11/2018 8:10:35 PM
On Thu, Jan 11, 2018 at 12:10:35PM -0800, Bobby Holley wrote:
>On Thu, Jan 11, 2018 at 12:06 PM, Kris Maglione <kmaglione@mozilla.com>
>wrote:
>> On Thu, Jan 11, 2018 at 05:12:37PM +0100, Tom Schuster wrote:
>>> This could be an issue for WebExtensions as well. I think the
>>> contentscript
>>> sandbox runs in a different compartment.
>>
>> It runs in a different compartment, but the DOM constructors it has access
>> to come from the same content window as the nodes it would be checking, so
>> this isn't an issue.
>
>Oh, I guess Xrays behavior would be unaffected by Cameron's proposal.
>Ignore my previous message.

Right, as long as both the node that's being checked and the 
constructor have X-ray wrappers, they work as expected without 
isinstance hooks (albeit much more slowly).

It might change the behavior when checking a non-X-rayed node 
against an X-rayed constructor, or vice versa, though. Hopefully 
people aren't relying on that.
0
Kris
1/11/2018 8:18:00 PM
Reply: