819131 - Cross-compartment DOM element references disappear from WeakMaps

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Dave Camp (:dcamp)]

Attachment: mochitest showing the behavior

... unless I use an expando to keep the wrapper alive.

This is a pain for some devtools work, we'd like to use WeakMaps against content nodes.

Comment 1

[Andrew McCreight [:mccr8]]

Bill, could this be a missing case for your CCW weakmap key stuff? Like maybe we don't handle CCWs for reflectors correctly or something annoying like that?

Comment 2

[Wladimir Palant]

This is bug 655297 regressed. Steps to reproduce:

* Go to about:config and make sure that devtools.chrome.enabled preference is set to true.
* Go to http://www.heise.de/ and press Shift-F4 to open ScratchPad.
* Open the Environment menu and select Browser.
* Paste the following code into the ScratchPad:

var weakMap = new WeakMap();
weakMap.set(content.document.body, "success");
window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
      .getInterface(Components.interfaces.nsIDOMWindowUtils)
      .garbageCollect();
alert(weakMap.get(content.document.body));

* Press Ctrl-R to run the code.

Expected results:

A message appears saying "success" (weak map entry still present).

Actual results:

A message appears saying "undefined" (weak map entry got garbage collected).

For some reason, the issue is only reproducible on some websites - e.g. on bugzilla.mozilla.org I still get "success". On other websites like heise.de it is 100% reproducible however. So far I tested Firefox 18 and Firefox 21.0a1 (2013-01-21 nightly) on OpenSUSE, both are affected. Will try to determine a regression range, I'm pretty certain that Firefox 17 didn't exhibit this issue.

Comment 3

[Wladimir Palant]

Regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb573b9307e5&tochange=0ff60bfb3442. Bill, please mark this bug as blocking bug 798678 - I don't have access. I assume that bug 798678 got merged into Firefox 18 despite landing during the Firefox 19 time frame.

Comment 4

[Alex Keybl [:akeybl]]

We don't think this needs to be tracked for upcoming releases given its lack of severity - if it slows devtools work, we can take a low risk uplift to appropriate branches once resolved.

Comment 5

[Andrew McCreight [:mccr8]]

Well, I was nominating for tracking because we broke AdBlock Plus, it looks like...

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

you'll need to renominate if you want that fact taken into consideration.

Comment 7

[Wladimir Palant]

Adblock Plus is using getUserData/setUserData again until this bug is fixed - the release is scheduled for next Tuesday. So this probably won't count as a serious reason.

Comment 8

[Andrew McCreight [:mccr8]]

Okay, thanks for the explanation.  Sorry for the headache here.

Bill, are you going to have time to look at this next week?  If not, I can try my hand at it.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think I'd be able to get to this until end of the week at the earliest. Any help is welcome :-).

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: preserve reflector delegates

As expected, the problem is that this key is a cross-compartment wrapper around a reflector.  I think the solution is to do the preserveWrapper callback dance on the delegate as well as the key.

I still need to write some more tests.  The original test (included here) at least passes.  I also need to try the STR from comment 2.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: preserve reflector delegates

https://tbpl.mozilla.org/?tree=Try&rev=026c21d3391b

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 709252 [details] [diff] [review]
preserve reflector delegates

Review of attachment 709252 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.

Comment 13

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffdee4a4eb7f

Comment 14

[Andrew McCreight [:mccr8]]

It would probably make sense to get this on Aurora.  ESR17 would be nice, but on the other hand weak maps didn't really work very well on ESR10 so this isn't much of a regression there.  So I guess for b2g18 and esr17 we should probably just let sleeping dogs lie.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ffdee4a4eb7f

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Andrew McCreight [:mccr8] from comment #14)
> It would probably make sense to get this on Aurora.  ESR17 would be nice,
> but on the other hand weak maps didn't really work very well on ESR10 so
> this isn't much of a regression there.  So I guess for b2g18 and esr17 we
> should probably just let sleeping dogs lie.

Could you nominate an approval on Aurora?

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 709252 [details] [diff] [review]
preserve reflector delegates

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 798678
User impact if declined: causes problems for some addons and maybe some websites
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 709252 [details] [diff] [review]
preserve reflector delegates

low risk, approving for Aurora uplift.

Comment 20

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c557b7c03a4

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

doesn't meet ESR uplift criteria, we'll get this in ESR24.