604523 - JSON.stringify(empty array from another scope) -> {}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ed Lee :Mardak]

Attachment: restartless add-on dumping to error console

function startup() {
  AddonManager.getAddonsByIDs([], function(a) {
    _("uneval", uneval(a));
    _("constructor", a.constructor);
    _("constructor == Array", a.constructor == Array);
    _("constructor == Array + ''", a.constructor == Array + '');
    _("json", JSON.stringify(a));
  });
}


results in..

[]
function Array() { [native code] }
false
true
{}

For non-empty arrays, it'll end up stringifying as {"0": "val", ...}

Comment 1

[Ed Lee :Mardak]

Fail minefield 20101014 http://hg.mozilla.org/mozilla-central/rev/ad0a0be8be74
Pass minefield 20101013 http://hg.mozilla.org/mozilla-central/rev/178f26e21cfc

Comment 2

[Ed Lee :Mardak]

This doesn't seem to affect code crossing iframes, so it might be only chrome code importing another script that returns an array.

I suppose one workaround is to manually copy the array into a local array and hope there aren't nested arrays.

Comment 3

[Ed Lee :Mardak]

This might be hitting ctypes used from restartless addons as well assuming the same issue of arrays looking like objects:

ctypes.StructType("foo", [..])

"Error: second argument must be an array"
http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#3990

Comment 4

[Ed Lee :Mardak]

Perhaps this is related to how the bootstrap code is run? Is the loading code based on other code that might be affected by the compartments change?

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#4130

They're created with..

createInstance(Ci.nsIPrincipal)
Components.utils.Sandbox(principal)
mozIJSSubScriptLoader.loadSubScript

Comment 5

[Andreas Gal :gal]

Attachment: patch

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

dvander: change to the method jit

vlad: typed arrays now accept everything with a length property

Comment 8

[Andreas Gal :gal]

(vlad, we could also use GetLengthProperty unconditionally, and if there is no length property, its treated like an array of length 0, what do you think?)

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 484587 [details] [diff] [review]
patch

r=me on methodjit part

Comment 10

[Ed Lee :Mardak]

content/base/src/nsContentUtils.cpp:5857: error: 'struct js::ClassExtension' has no member named 'wrappedObject'
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#5856

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 484587 [details] [diff] [review]
patch

Not sure about the lack of length property meaning 0... do other APIs behave like that?

Comment 12

[Andreas Gal :gal]

vlad: other array functions behave like that, so if you are ok with it I would duplicate that behavior

Comment 13

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/438afef93fd5

Comment 14

[Andreas Gal :gal]

Pushed with the behavior Array.prototype.* have. Will follow-up with a fix if vlad wants something else. This blocks b7 so I want to get it landed.

Comment 15

[Andreas Gal :gal]

Backed out due to suspected orange.

Comment 16

[Damon Sicore (:damons)]

Gal will fix test and land asap.

Comment 17

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/ab80a372cfe1

Comment 18

[Andreas Gal :gal]

Follow-up, removing a bogus assert.

http://hg.mozilla.org/tracemonkey/rev/6d88ce7787ad

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/139dcd10518f

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/eaf6f9566f2e (is the follow-up)