Closed Bug 714110 Opened 13 years ago Closed 13 years ago

SDK passes arrays into content scripts as objects

Categories

(Firefox :: Extension Compatibility, defect)

Product:
Firefox
Component:
Extension Compatibility
Version:
10 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox10 + ---

People

(Reporter: canuckistani, Assigned: ejpbruel)

References

Details

A difference in behaviour has been noticed in the Add-on SDK when run with Firefox 9 and earlier or 10 and later.

In 9 and earlier, and array that is passed to a content script from main.js using port.emit() preserves its type.

In 10 and later, the array instead end up as an object with string keys, eg

var a = ["one", "two", "three"]

ends up as 

{"0":"one","1":"two","2":"three"}

This issue was reported in the Jetpack google group:

https://groups.google.com/forum/#!topic/mozilla-labs-jetpack/kykwu4BdRv8

Here is a simple Builder example the illustrates the issue:

https://builder.addons.mozilla.org/addon/1031572/latest/

To reproduce, run the example in Firefox 9 and Firefox 10 Beta.
Ccing Eddy as this is a Fireofx / Platform issue, and he's our man in Amsterdam.
cc'ing the JS guys as well
Do we know how many/what kind of addons are affected? This seems like a blocking-level issue to me.
Will take a look at this when I get back from my week off.

Benjamin, what is a blocking-level issue?
Assignee: nobody → ejpbruel
That we have broken (some set of) SDK-based addons, so we would no-go Firefox 10 unless this were fixed.
To reproduce this bug:

1. install / download Firefox Beta / Aurora / Nightly
2. open this builder link and run the add-on:

https://builder.addons.mozilla.org/addon/1031572/latest/

When the add-on runs, it will fetch some JSON from the internet ( my dropbox account to be exact ) and pass it as a parsed object into a content script attached to a panel page. The panel page will display two things:

'Before' is pretty-printed output of the JSON as it existed in main.js. It will always look like this:

[
    {
        "users": [
            {
                "name": "dennis"
            },
            {
                "name": "george"
            },
            {
                "name": "bart"
            }
        ],
        "message": {
            "label": "Custom message goes here"
        }
    }
]

'After' is pretty-printed output of the object once it was passed via a port.emit() call into the content script. On Firefox <= 9.01, this output is identical to the output above. On versions of Firefox >= Firefox Beta 10, it instead looks like this:

{
    "0": {
        "users": {
            "0": {
                "name": "dennis"
            },
            "1": {
                "name": "george"
            },
            "2": {
                "name": "bart"
            }
        },
        "message": {
            "label": "Custom message goes here"
        }
    }
}
port.emit() is defined ( more or less ) here:

https://github.com/mozilla/addon-sdk/blob/release/packages/api-utils/lib/content/worker.js#L201-L206

I cc'd Matteo, Irakli and Alexandre on this bug, and am now cc'ing Myk and Warner as well. I'd like to hear their input as to where we use Firefox APIs that might be affected by this.
Here is the platform issue:
  var s = Cu.Sandbox("http://mozilla.org");
  var f = Cu.evalInSandbox("(function test(s) JSON.stringify(s))", s);
  dump( f([1, 2, 3]) +"\n");

On FF8: [1,2,3]
On Nightly (12.0a1 (2011-12-21)): {"0":1,"1":2,"2":3}

I didn't try with a content document, but I'd expect to get the same issue?
We may have some way to fix this issue on SDK side.
Ideally we will only transfer strings between compartments,
but that's still not the case.
[triage comment]
Tracking for Firefox 10. We don't want to ship something that breaks add-on SDK consumers.
tracking-firefox10: ? → +
Alex: I don't think the SDK should work around this, unless this was a deliberate change. In my mind JSON.stringify([1,2,3]) should never result in {"0":1,"1":2,"2":3}, that's just wrong!
(In reply to Jeff Griffiths from comment #11)
> Alex: I don't think the SDK should work around this, unless this was a
> deliberate change. In my mind JSON.stringify([1,2,3]) should never result in
> {"0":1,"1":2,"2":3}, that's just wrong!

Sure, it sounds like quite important platform bug here that needs to be fixed in anycase.
I just try to give some input about SDK internals and its future.
We may end up passing only strings between compartements for security reason, not only to fix this.
Currently we are serializing/parsing all objects to/from the content using JSON.parse and JSON.stringify, but we are doing both operations on chrome side, whereas we should stringify in emitter and only parse in receiver side.
So that both sides won't have any wrapper anymore. And I'm pretty sure it will avoid us to face similar wrapper issues in the future and may fix existing ones as we have various content script bugs opened!

So if it can ease things to do this in the SDK, we may work on this for 1.5 release.
I totally agree with Jeff.
As additional information, notice that only `JSON.stringify` seems broken: `uneval` and the method `toSource` still works across sandbox.
Consider the following code:

var s = Components.utils.Sandbox('http://www.mozilla.org')
var f = Components.utils.evalInSandbox('(function f(x)Array.isArray(x))', s);
f([1, 2, 3])

This yields true on FF8 but false on the Nightly.

Now consider the following code in json.cpp, line 605:

if (ObjectClassIs(v.toObject(), ESClass_Array, cx))
    ok = JA(cx, obj, scx);
else
    ok = JO(cx, obj, scx);

where v is the first argument that was passed to JSON.stringify. Since v is an Array, the call to ObjectClassIs should return true, but returns false instead. The implementation of ObjectClassIs has a special case for when the object being tested is a proxy, which is a call to Proxy::objectClassIs, but this returns the wrong result. The question is why.
Addendum: it looks like Proxy::objectClassIs always returns false, even if the underlying object really is an array. I don't see any comments that this function is currently not implemented so this probably intentional, but at the same time I cannot imagine that this behavior is correct. What's more, it makes me wonder what changed between FF8 and Nightly that caused this to work earlier?
The following changeset introduced the the problem (see bug 690825):
https://hg.mozilla.org/mozilla-central/rev/949c2cf4c772
 
This changeset introduced a new type of secure cross-compartment wrapper. Among other things, this wrapper prevents scripts with content privileges from inspecting certain properties of objects that originate from a script with chrome privileges. In this particular example:

var s = Components.utils.Sandbox('http://www.mozilla.org')
var f = Components.utils.evalInSandbox('(function f(x)Array.isArray(x))', s);
f([1, 2, 3])

the function f is not allowed to check the type of the object [1, 2, 3], which causes the call to isArray to return an erroneous result (false, where it should return true).

However, the above behavior is thus not a bug, but very much intentional. This is a bit problematic, since we can't just back out the changes introduced by this commit, but leaving them in could break compatibility with existing add-ons.

After talking it over with the folks in #jsapi, the consensus seems to be that we shouldn't pass anything other than strings between compartments to begin with (in fact, since object wrappers are extremely hard to get right, it's very well possible that this might be the only form of inter compartment communication we'll end up allowing in the end). This was already the workaround suggested by Alex in comment 9, so in my opinion we should just go for that.

That leaves open the possibility that the situation as it is might break compatibility with existing add-ons. We might just have to take our losses here, since I don't see any obvious way around this. One possibility could be to throw an exception whenever a call to Array.isArray or JSON.stringify fails due to security reasons, but this would still require people to rewrite their add-ons, and doing so would probably have implications that I'm not aware of that render such a solution impossible.

All in all, I'm leaning towards marking this issue as WONTFIX, unless anybody else is willing to offer some insight into this.
In my previous comment I said that the behavior we're seeing is intentional, but what is not clear to me is why being able to check the type of an object defined in a script with chrome privileges from a script with content privileges is a security risk. If we are going to end up breaking existing add-ons with this, I would at least like to see a very strong case why we absolutely need to disallow checking the type of an object that is passed into a compartments. Could anybody shed some light on this?

P.s. I'm all for the rule that we should only communicate between compartments with strings, as this would avoid a whole lot of issues, but we still have to keep backwards compatibility in mind
Blocks: 690825
We've decided to WONTFIX this bug during the weekly meeting of the addon-sdk team. I've opened a new bug for the proposed workaround in the addon-sdk itself (see bug 714891)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.