The default bug view has changed. See this FAQ.

Arrays returned by evalInSandbox aren't instanceof Array

RESOLVED FIXED in Firefox 3 alpha1

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Simon Bünzli, Assigned: Simon Bünzli)

Tracking

Trunk
Firefox 3 alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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
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;
(Assignee)

Comment 2

11 years ago
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.
Attachment #240121 - Flags: review?(mano)

Comment 3

11 years ago
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 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...
(Assignee)

Comment 5

11 years ago
(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 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...
(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 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.
Attachment #240121 - Flags: review?(mano) → review+
(Assignee)

Comment 9

11 years ago
Created attachment 241655 [details] [diff] [review]
fix for SessionStore (nits addressed)
Assignee: general → zeniko
Attachment #240121 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.51
Assignee: zeniko → nobody
Status: ASSIGNED → NEW
Component: JavaScript Engine → Tabbed Browser
Flags: review+
Product: Core → Firefox
QA Contact: general → tabbed.browser
Whiteboard: [checkin needed]
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 alpha1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: nobody → zeniko
Status: REOPENED → NEW
oops!
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

10 years ago
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?
(Assignee)

Comment 13

9 years ago
As per bug 402349 comment #6 landing this on the branch would fix a nasty regression introduced with the fix for bug 367605.
Blocks: 402349
Flags: blocking1.8.1.12?
(Assignee)

Updated

9 years ago
Attachment #241655 - Flags: approval1.8.1.12?
(Assignee)

Comment 14

9 years ago
Solving this in bug 402349.
Flags: blocking1.8.1.12?
(Assignee)

Updated

9 years ago
Attachment #241655 - Flags: approval1.8.1.12?
You need to log in before you can comment on or make changes to this bug.