Opened on 03/11/2014 at 03:28:33 PM
Closed on 05/18/2014 at 05:04:56 PM
Last modified on 06/30/2014 at 09:15:33 AM
#47 closed defect (fixed)
Get rid of deprecated getUserData() method
| Reported by: | saroyanm | Assignee: | saroyanm |
|---|---|---|---|
| Priority: | P2 | Milestone: | Adblock-Plus-2.6.2-for-Firefox |
| Module: | Adblock-Plus-for-Firefox | Keywords: | externaldependency |
| Cc: | trev | Blocked By: | |
| Blocking: | Platform: | ||
| Ready: | yes | Confidential: | no |
| Tester: | Verified working: | no | |
| Review URL(s): | |||
Description (last modified by trev)
Firefox Nightly currently warns about us using getUserData() and setUserData() in requestNotifier.js, the following message appears for pretty much every page:
Use of getUserData() or setUserData() is deprecated. Use WeakMap or element.dataset instead. requestNotifier.js:64
Attachments (0)
Change History (22)
comment:2 Changed on 03/11/2014 at 08:58:16 PM by trev
- Description modified (diff)
- Summary changed from Fix deprecated warning in Firefox Nightly to Get rid of deprecated getUserData() methid
comment:4 Changed on 03/11/2014 at 09:11:12 PM by trev
- Cc trev added
comment:5 Changed on 03/11/2014 at 09:39:48 PM by trev
- Summary changed from Get rid of deprecated getUserData() methid to Get rid of deprecated getUserData() method
comment:6 Changed on 03/12/2014 at 10:09:34 AM by trev
Manvel filed a Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=982561
comment:7 Changed on 03/12/2014 at 10:26:11 AM by trev
- Keywords externaldependency added
comment:8 Changed on 03/12/2014 at 11:57:55 AM by philll
- Owner set to saroyanm
- Status changed from new to assigned
comment:9 Changed on 03/12/2014 at 10:22:15 PM by fhd
- Priority changed from Unknown to P4
- Reporter changed from philll to saroyanm
comment:10 Changed on 03/31/2014 at 09:38:52 AM by philll
- in_progress set to 0
- Priority changed from P4 to P2
- Ready unset
I've raised the priority a bit as FF28 is stable already and pollutes the console with that warning.
comment:11 Changed on 03/31/2014 at 10:16:44 AM by trev
Priority is pretty irrelevant here given that we are blocked by a Mozilla bug - as it is now our only options are producing this warning or breaking our functionality.
comment:12 Changed on 04/13/2014 at 01:13:07 PM by tschuster
Interesting, I will take a look at the Mozila bug, maybe I can fix it.
comment:13 Changed on 04/14/2014 at 05:48:59 AM by trev
I've updated the comment in our source code to list bug 982561 as yet another blocker for WeakMaps: https://hg.adblockplus.org/adblockplus/rev/639110e5915d
comment:14 Changed on 04/30/2014 at 09:50:33 AM by philll
- Status changed from assigned to new
The assigned state will be dropped by #403
comment:15 Changed on 05/09/2014 at 07:04:41 AM by trev
- Owner saroyanm deleted
Unassigning - I don't think this is currently being worked on.
comment:16 Changed on 05/18/2014 at 12:09:12 PM by saroyanm
Good news:
The issue with WeakMaps looks to be fixed.
I've tested with FF nightly (32.0a1), with our Scratchpad script and also made manual test, seams that the issue is gone.
Maybe make sense to have case that will check for FF Version 32 and newer and enable WeakMaps for that case.
I'll create a review for that.
After the review I guess we can confirm to Mozilla community that the issue was fixed on our side.
comment:17 Changed on 05/18/2014 at 12:13:11 PM by saroyanm
- Owner set to saroyanm
comment:18 Changed on 05/18/2014 at 12:53:01 PM by saroyanm
- Review URL(s) modified (diff)
comment:19 Changed on 05/18/2014 at 05:04:56 PM by saroyanm
- Resolution set to fixed
- Status changed from new to closed
comment:20 Changed on 05/26/2014 at 01:34:53 PM by trev
- Milestone set to Adblock-Plus-for-Firefox-next
- Ready set
comment:21 follow-up: ↓ 22 Changed on 06/30/2014 at 09:02:59 AM by 2nd
For me this line causes same error https://hg.adblockplus.org/adblockplus/file/ce3d000d9178/lib/requestNotifier.js#l63. Very annoying.
Firefox/28.0
comment:22 in reply to: ↑ 21 Changed on 06/30/2014 at 09:15:33 AM by trev
Replying to 2nd:
For me this line causes same error https://hg.adblockplus.org/adblockplus/file/ce3d000d9178/lib/requestNotifier.js#l63. Very annoying.
Firefox/28.0
Of course it does - the issue is only resolved in Firefox 32 and above. Earlier versions are still affected by bug 982561.
There is a review for this with some discussion: http://codereview.adblockplus.org/5112750644658176/
The essence of it is: we have a code path using WeakMap instead of setUserData(). However, we cannot currently use it because WeakMap will lose data occasionally. This is reproducible on http://winfuture.de/ - with the WeakMap code path, if you wait a few minutes after opening that page right-clicking the logo will no longer offer you to block it, the corresponding URL will also be missing in the list of blockable items along with a number of others. This is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=819131 but triggering garbage collection manually doesn't seem to trigger this issue.
What we need is a reliable way to associate some data with an element. It is important that we don't keep a reference to this element - if the element is removed from DOM it should be garbage collected, not kept around. Same goes for our data, it should only stay in memory for as long as the element it belongs to. Finally, it needs to be out of reach for webpage scripts. For this, setUserData() and WeakMap seem to be the only options. Note that expando properties don't work because these are set on an XPCNativeWrapper which can (and will) be garbage collected independently of the actual DOM element it refers to.
Manvel was working on a simplified test case for this that we could submit as a Mozilla bug.