372102 - ChatZilla is a bit broken on current Firefox trunk

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, defect)

Description

[:Gijs (he/him)]

Because Firefox runs with xpcnativewrappers on (seamonkey doesn't, and we're not broken there), which now come with extra wrappers (bug 355766), and because we switched to content-type <browser>s, we're now having all kinds of strange problems:

[ERROR] Internal error dispatching command “set-current-view”.
[ERROR] Permission denied to get property ChromeWindow.Event 

and

[ERROR] Internal error dispatching command “attach”.
[ERROR] NS_ERROR_ILLEGAL_VALUE: Illegal value @ <chrome://chatzilla/content/static.js> 3209 

amongst other things.

They seem to mostly revolve around our importFromFrame bits. Boris Zbarsky explained to me:

> function importFromFrame(method)
> {
>     client.__defineGetter__(method, import_wrapper);
> ....
>     return window[method];
> }
> 
> Now |window| is an untrusted window in this case, so you get back an SJOW
> [XPCSafeJSObjectWrapper] for it, and hence window[method] is an SJOW for the
> method.  Currently, you _cannot_ call an SJOW for a method with a |this| which 
> is not an SJOW itself.  This is pretty much by-design.  So the call to
> updateHeader throws.
> 
> Do the various methods you're importing depend on the |this| being the thing
> they're imported to?  If so, could you make that object an SJOW itself?  That
> would help... If not, change the importing to call the methods with the window
> as the |this|? 

So it looks to me as if we should come up with a different scheme for this. I checked output-window.js, it doesn't look as if anything actually uses |this|, so it won't matter what we call it on, I think? Maybe there are scope issues, though. I will try to poke this tonight. If anyone beats me to it, be my guest.

Comment 1

[James Ross]

The error is abysmal, FWIW, and should be fixed.

What I'm confused about is why is wrappedJSObject returning a fake object? Everything I've seen and heard about it says it is the real JS object, but it's clearly an XPC object faking it, in this case at least.

Considering I'd never heard of XPCSafeJSObjectWrapper until my debugger stopped in it last night, there's something major that's not been explained about this. (I'm hoping bug 355766 will explain, when I've read all the comments, but I don't hold much hope, even if it is the only reference anywhere to the thing.)

Also, BECAUSE it was faking it - right down to the toString - it's impossible to tell unless you know what bugs it has, like typeof fakeThing.func == "object" instead of "function".

Comment 2

[James Ross]

Attachment: Exploit loophole in safeJSObject

This causes it to call the 'safe wrapped' function with the right this, whilst not getting an error from safeJSObject. If window[method] is a normal JS function (no wrappers in play), it still works fine.

Comment 3

[:Gijs (he/him)]

Comment on attachment 256803 [details] [diff] [review]
Exploit loophole in safeJSObject

I'm not sure what our style rules are for functions you're returning... otherwise, the brace for import_wrapper_apply() should be on the next line. I'll leave that up to you.

r=gijs

Comment 4

[James Ross]

Comment on attachment 256803 [details] [diff] [review]
Exploit loophole in safeJSObject

Checked in, with the brace moved.

Comment 5

[James Ross]

We've identified the bug in SafeJSObject (SJOW) in bug 355766, and a fix for it is being done there. This bug is done now. We might want to revisit the code when that fix lands, but there's no need to keep this open.

Comment 6

[Daniel Veditz [:dveditz]]

Reading this bug this is a regression from SJsOW, and this fix would be required on the 1.8 branch if we took SJsOW there, right?

Comment 7

[James Ross]

As long as the bug with SJsOW that we found via ChatZilla is fixed as well (attachment from bug 355766 comment 36), it shouldn't matter to us. If that bug hadn't existed in the original SJsOW implementation, this bug would never have happened. :)