Closed Bug 819131 Opened 7 years ago Closed 7 years ago

Cross-compartment DOM element references disappear from WeakMaps

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 - wontfix
firefox20 - fixed
firefox21 - fixed
firefox-esr17 --- wontfix
b2g18 --- affected
b2g18-v1.0.0 --- affected
b2g18-v1.0.1 --- affected

People

(Reporter: dcamp, Assigned: mccr8)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Attached patch mochitest showing the behavior (obsolete) — Splinter Review
... 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.
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?
Assignee: nobody → wmccloskey
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.
Blocks: 655297, abp
Keywords: regression
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.
Blocks: 749981
Blocks: 798678
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.
Well, I was nominating for tracking because we broke AdBlock Plus, it looks like...
you'll need to renominate if you want that fact taken into consideration.
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.
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.
Flags: needinfo?(wmccloskey)
I don't think I'd be able to get to this until end of the week at the earliest. Any help is welcome :-).
Flags: needinfo?(wmccloskey)
Assignee: wmccloskey → continuation
Attached patch preserve reflector delegates (obsolete) — Splinter Review
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.
Attachment #689424 - Attachment is obsolete: true
Attachment #709252 - Flags: review?(wmccloskey)
Comment on attachment 709252 [details] [diff] [review]
preserve reflector delegates

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

Thanks for fixing this.
Attachment #709252 - Flags: review?(wmccloskey) → review+
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/ffdee4a4eb7f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(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 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
Attachment #709252 - Flags: approval-mozilla-aurora?
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
Comment on attachment 709252 [details] [diff] [review]
preserve reflector delegates

low risk, approving for Aurora uplift.
Attachment #709252 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
doesn't meet ESR uplift criteria, we'll get this in ESR24.
Blocks: WeakMap
You need to log in before you can comment on or make changes to this bug.