500931 - xpcIJSWeakReference.get() unwraps XPCNativeWrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Wladimir Palant]

If you access an unsafe object from an extension it gets wrapped automatically:

  dump(doc);   // prints "[object XPCNativeWrapper [object HTMLDocument]]"

However, if you get a weak reference to this object and then get the value - the wrapper is gone:

  doc = Components.utils.getWeakReference(doc);
  dump(doc.get());   // prints "[object HTMLDocument]"

This didn't happen in Firefox 3.0.11 but I see this behavior in both trunk and 3.5 nightlies.

Comment 1

[Wladimir Palant]

Regression range on trunk is 44d20da17412 to f9e5c69f3a93: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44d20da17412&tochange=f9e5c69f3a93

Comment 2

[Wladimir Palant]

Regression range on 1.9.1 branch is c11e41845954 to 5b61f163f2fd: http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=c11e41845954&tochange=5b61f163f2fd

This should be caused by bug 475864 then.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

This was nominated to block the final release of Firefox 3.5 without any rationale; I assume that was just flag tweaking to get attention? Renominate if you have a reason why we should stop-ship on Firefox 3.5 for this issue.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I could take the thisObject hook and make the relevant code a static function somewhere, but this seemed easier. Explanation in the comment in the patch.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 385866 [details] [diff] [review]
Proposed fix

>+    if (!cx) return NS_ERROR_UNEXPECTED;

Ugh, let's get this on two lines. And the other instance too!

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Cleaned up

This has a unit test, it removes the stray bit of JS in browser.js I was using to test with and it addresses bent's review comments. Carrying forward his review...

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/291552e67a3b

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Backed out due to leaks.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

For the record, the leaks involved have nothing to do with the patch. Somehow, xpcJSWeakReferences cause leaks and the patch makes this a problem because it adds a (chrome) mochitest that tests them.

Comment 10

[Wladimir Palant]

Confirmed, xpcIJSWeakReference causes memory leaks. Not blocking Adblock Plus any more, I am using nsISupportsWeakReference directly now which doesn't exhibit any issues in current releases.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/d30b5dd62042

Comment 12

[Samuel Sidler (old account; do not CC)]

Not *blocking* 1.9.1.1, but we'll consider a patch and it might block 1.9.1.2, if we don't take it in 1.9.1.1.

Comment 13

[Samuel Sidler (old account; do not CC)]

Blake: Any reason this should block? Definitely wanted, but sell me on the blocking.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 385880 [details] [diff] [review]
Cleaned up

I don't know if I can sell blocking, but as long as this lands, I don't mind.

Comment 15

[Samuel Sidler (old account; do not CC)]

Comment on attachment 385880 [details] [diff] [review]
Cleaned up

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 and use the ".2-fixed" option of the "status1.9.1" flag.

We don't need this on 1.9.0, right?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b1e2ea0373a0

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

We probably want it on 1.9.0.

Comment 18

[Tony Chung [:tchung]]

Please advise on how this can be verified on fx3.5.2.  Thanks.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Tony: Can you use the fact that there is a mochichrome test that passes on tinderboxes to verify this? If not, Wladimir might be the best person.

Comment 20

[Tony Chung [:tchung]]

thats good enough for me.  marking this verified1.9.1

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to comment #17)
> We probably want it on 1.9.0.

Probably? or really? will take your approval1.9.0.14? request on a patch as the answer.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 385880 [details] [diff] [review]
Cleaned up

I was not sure because I was seeing leaks with both this patch and the patch for bug 501577 applied. But I'm seeing those leaks without either of these patches, so this isn't the cause. This should be ready to go.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 385880 [details] [diff] [review]
Cleaned up

Approved for 1.9.0.14, a=dveditz for release-drivers

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Verifying it for 1.9.0.14 based on passing test.