Closed Bug 819131 Opened 7 years ago Closed 7 years ago
Cross-compartment DOM element references disappear from Weak
... 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.
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.
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.
I don't think I'd be able to get to this until end of the week at the earliest. Any help is welcome :-).
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 #708748 - 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+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
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+
You need to log in before you can comment on or make changes to this bug.