Last Comment Bug 350558 - Arrays returned by evalInSandbox aren't instanceof Array
: Arrays returned by evalInSandbox aren't instanceof Array
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 3 alpha1
Assigned To: Simon Bünzli
:
Mentors:
Depends on:
Blocks: 402349
  Show dependency treegraph
 
Reported: 2006-08-29 01:02 PDT by Simon Bünzli
Modified: 2007-12-06 08:24 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix for SessionStore (3.28 KB, patch)
2006-09-26 02:51 PDT, Simon Bünzli
no flags Details | Diff | Review
fix for SessionStore (nits addressed) (3.29 KB, patch)
2006-10-08 16:32 PDT, Simon Bünzli
no flags Details | Diff | Review

Description Simon Bünzli 2006-08-29 01:02:42 PDT
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 [:bz] 2006-09-21 21:38:59 PDT
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 2006-09-26 02:51:34 PDT
Created attachment 240121 [details] [diff] [review]
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 2006-09-26 03:02:18 PDT
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 [:bz] 2006-09-26 07:40:10 PDT
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 2006-09-26 08:24:49 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-27 16:36:17 PDT
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, jst@mozilla.com) 2006-09-27 17:35:26 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-10-08 16:10:33 PDT
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 2006-10-08 16:32:50 PDT
Created attachment 241655 [details] [diff] [review]
fix for SessionStore (nits addressed)
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-10-10 06:46:57 PDT
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.51
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-10-10 15:50:14 PDT
oops!
Comment 12 Simon Bünzli 2007-01-15 05:46:07 PST
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 2007-11-29 09:02:15 PST
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 2007-12-06 08:24:10 PST
Solving this in bug 402349.

Note You need to log in before you can comment on or make changes to this bug.