613515 - JS properties set from chrome are lost

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mark Smith [:mcs]]

I mentioned this problem in bug 601803... I have an extension that assigns a JS Object to a property on a content element.  In Firefox 4.0b7 and in current trunk builds, the object cannot be retrieved reliably from within the extension (the value is undefined).

Usually, the object can be retrieved for at least a few seconds (or several retrievals) before undefined is returned.  After undefined is returned the first time, subsequent retrievals always return undefined.

We created a small extension that demonstrates the problem and we will post it here shortly.

This problem does not occur in Firefox 3.6.12 or in Firefox 4.0b6.

Comment 1

[Mark Smith [:mcs]]

Attachment: extension that demonstrates the bug

To see the problem, install this extension.  After page load, it adds an image element to the page and assigns a JS Object to the element in a property named MYPROP.  It then starts a timer that checks MYPROP every second, displaying an alert when undefined is returned.  Click the image to attempt to access MYPROP manually.

We added the timer to make it easier to see the problem; without the timer, the problem will still occur if you click the image enough times.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sounds like we don't gc-mark xray wrappers from the wrapped object?

Comment 3

[Andreas Gal :gal]

If #2 is correct, we should hide the bug.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This shouldn't be a security bug. No GC hazard here. All this means is that expando properties get lost when the proxy gets recreated when the object is next touched from chrome.

Comment 5

[Mark Smith [:mcs]]

(In reply to comment #4)
> All this means is that expando properties get lost when the proxy gets
> recreated when the object is next touched from chrome.

So does that mean this is a valid bug?  Or is this "by design"?  If this is a valid bug, is it something that is likely to be fixed in an upcoming beta?

Comment 6

[Brendan Eich [:brendan]]

This is a valid bug.

/be

Comment 7

[Benjamin Smedberg]

Blake, I thought that we were going to disallow setting custom properties on xray wrappers altogether. Did that not actually work out?

Comment 8

[Johnny Stenback (:jst)]

We need to sort this out one way or another before we ship.

Comment 9

[Johnny Stenback (:jst)]

Andreas, mrbkap says you could fix this one. Over to you to offload mrbkap a bit.

Comment 10

[Andreas Gal :gal]

Attachment: patch

Comment 11

[Andreas Gal :gal]

back to blake for debugging

Comment 12

[Andreas Gal :gal]

We are still seeing some rather strange failures so it will probably take another few days.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for that

This patch applies on top of attachment 503712 [details] [diff] [review] and fixes the testcase here. I think that with these two patches, we can land this!

Comment 14

[Johnny Stenback (:jst)]

The two attached patches combined cause test failures and leaks, specifically on the browser chrome test browser/base/content/test/browser_inspector_tab_switch.js.

And I also see:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/extensions/regress-369696-02.js | No test results reported. (SCRIPT)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-417893.js | No test results reported. (SCRIPT)

on all platforms on try.

Andreas, can you look into this more? Blake has his plate pretty full atm.

I'll attach the output of a browser chrome test run w/o this set of patches (works.log) and with (fails.log).

Comment 15

[Johnny Stenback (:jst)]

Attachment: works.log

Comment 16

[Johnny Stenback (:jst)]

Attachment: fails.log

Comment 17

[Andreas Gal :gal]

Meh, leaks are my least favorite bugs to look at.

Comment 18

[Johnny Stenback (:jst)]

It is certainly possible that the leak is due to a failure in the test causing something to not be properly torn down. So maybe the best approach here is to fix the errors first (the jsreftest ones), and then check whether the leaks remain or not, etc. I can help test if you have something resembling a fix for some of the errors...

Comment 19

[Andreas Gal :gal]

Yeah, good point.

Comment 20

[Andreas Gal :gal]

jst, neither reftest has anything to do with wrappers. Are you sure no other patches were involved when you pushed?

Comment 21

[Johnny Stenback (:jst)]

I don't see anything else that could cause the tests, the push was based on a revision that didn't show any problems on m-c.

The try results are at:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=e3650aaf3792

and the m-c results for the commit which mine was based on are at:

http://tbpl.mozilla.org/?rev=b22bb39e834f

I'll try to reproduce the js test failures locally to confirm.

Comment 22

[Andreas Gal :gal]

I tried a local shell reftest run. If this is some mean GC bug, maybe we mess up an unrelated test in the browser?

Comment 23

[Andreas Gal :gal]

jst can reproduce this

Comment 24

[Johnny Stenback (:jst)]

Attachment: Additional fix from Andreas.

This fixes the issues seen in the last push to try. This is from Andreas, r=mrbkap.

Comment 25

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/5a6e9e7e487a

Comment 26

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/5a6e9e7e487a

Comment 27

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre