870423 - `document instanceof HTMLDocument` returns false in a sandbox

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Simon Chan]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:23.0) Gecko/20130509 Firefox/23.0
Build ID: 20130509031047

Steps to reproduce:

1. Run following code on chrome of main browser window using an addon called JSX version 0.3
gBrowser.addEventListener("DOMContentLoaded", function (e) {
    var document2 = e.target;
    alert(document2);
    alert(document2 instanceof HTMLDocument);
});
2. Open a new tab and navigate to a (any) HTML page.


Actual results:

Two popups said:
[object HTMLDocument]
false


Expected results:

Two popups said:
[object HTMLDocument]
true

In Addition:
1. Firefox 20.0.1 works correctly (with same profile).
2. It is like bug #97939
3. As in bug #97939 ,use `alert(document2 instanceof document2.defaultView.HTMLDocument)` returns `true`

Comment 1

[Alice0775 White]

Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e500bc167b4e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130504 Firefox/23.0 ID:20130504170147
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1a60a6e9d9b8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130504 Firefox/23.0 ID:20130504184447
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e500bc167b4e&tochange=1a60a6e9d9b8

Suspected  Bug 855971

Comment 2

[Boris Zbarsky [:bzbarsky]]

> using an addon called JSX version 0.3

Seems to not work on non-Windows?  It tells me "Not available for your platform".

Given that, I can't tell you exactly why it's broken: it really depends on exactly how the addon is running this code and what it sets HTMLDocument to in that scope.  I just tried injecting the script above directly into browser.js, and it works just fine....

Comment 3

[Alice0775 White]

Alternate STR
1. Open Scratchpad and turn on chrome privilege
2. Paste the code
3. Run the code
4. Open web page

Actual Results:
returns "false" in Bad build
returns "true" in Good build.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, I found simpler steps to reproduce that we'll hope are equivalent:

1)  Set "devtools.chrome.enabled" to true in about:config
2)  Tools > Web Developer > Scratchpad
3)  Environment > Browser
4)  Type "content.document instanceof HTMLDocument"
5)  Execute > Display

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, that's what comment 3 is doing, yes.  ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

So using those STR, when we get into js::HasInstance, our JSClass* is a proxy but has fun_hasInstance as its class hasInstance hook.

And the reason for that is that js::FunctionProxyClass has:

3200     FunctionClass.hasInstance,

as its hasInstance hook.

In this case, our proxy handler is xpc::SandboxCallableProxyHandler.

I can't tell whether this is a bug in function proxies or in the sandbox WrapCallable thing, exactly...  Let's start with the latter.

Oh, and this used to work because HTMLDocument wasn't callable, presumably....  There's nothing HTMLDocument-specific here; it fails for any WebIDL interface object. 

I wonder whether we should just avoid the sandbox WrapCallable junk for things that are callable but have the webidl interface/proto class?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #6)
 
> I can't tell whether this is a bug in function proxies or in the sandbox
> WrapCallable thing, exactly...  Let's start with the latter.

I don't think it's the former - it seems like very reasonable behavior. If it's a pure proxy, then it seems like examining the prototype chain of the proxy is all we can do (short of introducing a new BaseProxyHandler trap for this operation). And if it's a wrapper, then it's up to the wrapper to set up its prototype chain to appropriately mirror the other side.

In this case, the obvious bug is that we pass nullptr as the prototype to js::NewProxyObject in WrapCallable. We should probably be passing the prototype of |callable|.
 
> I wonder whether we should just avoid the sandbox WrapCallable junk for
> things that are callable but have the webidl interface/proto class?

That seems kind of hacky. I'd prefer the former unless there are other problems with it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> If it's a pure proxy, then it seems like examining the prototype chain of the proxy is
> all we can do

Or proxying along to the hasInstance of the underlying object?

> short of introducing a new BaseProxyHandler trap for this operation

You mean like this trap already on BaseProxyHandler:

136     virtual bool hasInstance(JSContext *cx, HandleObject proxy, MutableHandleValue v, bool *bp);

?  ;)

> We should probably be passing the prototype of |callable|.

That's not good enough.  Binding interface objects have a very nontrivial hasInstance JSClass hook that does something totally different from walking the prototype chain.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> > If it's a pure proxy, then it seems like examining the prototype chain of the proxy is
> > all we can do
> 
> Or proxying along to the hasInstance of the underlying object?

Well, but pure proxies don't really have a notion of an underlying object. They have a call trap, but it's up to the handler to implement IIRC.

> 136     virtual bool hasInstance(JSContext *cx, HandleObject proxy,
> MutableHandleValue v, bool *bp);
> 
> ?  ;)

Oh, you're right. Then yeah, we should be using that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, so we should be creating our own proxy with our own handler then or something?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> OK, so we should be creating our own proxy with our own handler then or
> something?

No, SandboxCallableProxyHandler is a js::Wrapper, so it should get the appropriate forwarding behavior. We just need FunctionProxy to actually use the hasInstance hook instead of what it's doing now.

Comment 12

[Boris Zbarsky [:bzbarsky]]

That sounds like a JS engine issue...

Comment 14

[Bobby Holley (:bholley)]

Attachment: Use the actual hasInstance proxy hook for function proxies. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 16

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=beccd6aaba09

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 753099 [details] [diff] [review]
Tests. v1

r=me

Comment 18

[Bobby Holley (:bholley)]

Looks green.

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/97a87f7846af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5552b826c1

Comment 20

[Boris Zbarsky [:bzbarsky]]

We probably want this on Aurora too...

Comment 21

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #20)
> We probably want this on Aurora too...

I don't have a good picture of the regression story here. Can you nom?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm.  On Aurora, isn't web console still a sandbox?

Comment 23

[Boris Zbarsky [:bzbarsky]]

I guess maybe it's not. In that case the biggest issue would be extension compat, I assume.

Comment 24

[Boris Zbarsky [:bzbarsky]]

And I don't have a good feel for regression risk here, so have a hard time writing up the nom.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #24)
> And I don't have a good feel for regression risk here, so have a hard time
> writing up the nom.

So what's the regression concern, exactly? That code running in a sandbox will fail |document instanceof HTMLDocument|? Presumably this only matters for document, since we've done the dumb thing for proxy_HasInstance since the beginning of time?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97a87f7846af
https://hg.mozilla.org/mozilla-central/rev/0f5552b826c1

Comment 27

[Boris Zbarsky [:bzbarsky]]

> That code running in a sandbox will fail |document instanceof HTMLDocument|? 

Yes.

> Presumably this only matters for document

Well, it matters for all the various things we converted in the 23 cycle, but HTMLDocument is the one that people ran into.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Uplift to Aurora?

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 753098 [details] [diff] [review]
Use the actual hasInstance proxy hook for function proxies. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Pre-existing, but potentially exacerbated by bug 803542.
User impact if declined: |document instanceof HTMLDocument| will fail in the web console.
Testing completed (on m-c, etc.): Bake to m-c. 
Risk to taking this patch (and alternatives if risky): No alternatives. medium-low risk. 
String or IDL/UUID changes made by this patch: None.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2fe7df6517a
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7b87ea49450