350558 - Arrays returned by evalInSandbox aren't instanceof Array

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Simon Bünzli]

Steps to reproduce:
* Evaluate the following line (e.g. in the Error Console):

var a = Components.utils.evalInSandbox("[]", new Components.utils.Sandbox("about:blank")); a instanceof Array

Expected result:
|true|

Actual result:
|false|

NB: |a| still behaves like an array and the following holds true:
eval(uneval(a)) instanceof Array

Comment 1

[Boris Zbarsky [:bzbarsky]]

The array is an instance of the sandbox's Array, not the window's Array....  I'm not sure whether we want to (or can) change that.

That said, this is the same issue as an Array being created in one window not being instanceof Array from another window, no?  The solution, I would think, is to use Array from the right place like so:

  var sandbox = new Components.utils.Sandbox("about:blank");
  var a = Components.utils.evalInSandbox("[]", sandbox);
  a instanceof sandbox.Array;

Comment 2

[Simon Bünzli]

Attachment: fix for SessionStore

Since this seems to be no bug at all, here's the fix for SessionStore (where I came across the issue). This happens to require us to keep a sandbox around all the time - I just hope that this isn't much of a resource issue...

(In reply to comment #1)
> The array is an instance of the sandbox's Array, not the window's Array....

While this now seems quite obvious, it wasn't at first glance at all. Documenting this fact probably wouldn't hurt.

Comment 3

[Nickolay_Ponomarev]

I added a link to this bug to the http://developer.mozilla.org/en/docs/Components.utils.evalInSandbox page a while ago, feel free to replace it with an in-place note in docs.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 240121 [details] [diff] [review]
fix for SessionStore

Er.. do you seriously want to evaluate everything in a single sandbox?  Won't the various pieces of JS interfere with each other?

This patch and ensuing comments probably belongs in a separate bug...

Comment 5

[Simon Bünzli]

(In reply to comment #4)
> do you seriously want to evaluate everything in a single sandbox?

All we evaluate are toSource'd hashes and arrays - no state required; so a single sandbox should be fine. Using multiple sandboxes would BTW have the issue, that we'd have to keep all of them around, since we'd never know which sandbox the Array came from (or always uneval+evalInSandbox before we do the check).

> This patch and ensuing comments probably belongs in a separate bug...

Sure. OTOH I filed this bug because of this specific issue - and since there's apparently no underlying problem, we could just as well morph this bug into a one about getting the proper fix into place and be done with it.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 240121 [details] [diff] [review]
fix for SessionStore

Ugh, cannot you do something like if (typoeof(foo) == "object" && foo.constructor.name == "Array") instead, this is really a hack, but at least it doesn't require keeping a sandbox around...

Comment 7

[Johnny Stenback (:jst)]

(In reply to comment #6)
> (From update of attachment 240121 [details] [diff] [review] [edit])
> Ugh, cannot you do something like if (typoeof(foo) == "object" &&
> foo.constructor.name == "Array") instead, this is really a hack, but at least
> it doesn't require keeping a sandbox around...

No, accessing foo.constructor if foo came from a sandbox is not safe, it could do bad things...

Comment 8

[Asaf Romano (gone)]

Comment on attachment 240121 [details] [diff] [review]
fix for SessionStore

Ok :( This will do for now. but please use a variable instead of a constant, and rename it to EVAL_SANDBOX so no one thinks of firing anything else there, sigh.

r=mano.

Comment 9

[Simon Bünzli]

Attachment: fix for SessionStore (nits addressed)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.51

Comment 11

[:Gavin Sharp [email: [e-mail]]]

oops!

Comment 12

[Simon Bünzli]

So this is biting the first extension author ( http://forums.mozillazine.org/viewtopic.php?p=2698900#2698900 ) and it's kinda hacky to work-around (with the exception of the _closedTabs array, all arrays have to be manually converted from hashes if needed). Do we want to do anything about this on the 1.8.1 branch?

Comment 13

[Simon Bünzli]

As per bug 402349 comment #6 landing this on the branch would fix a nasty regression introduced with the fix for bug 367605.

Comment 14

[Simon Bünzli]

Solving this in bug 402349.