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:1 Changed on 03/11/2014 at 03:28:58 PM by philll

  • Description modified (diff)

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:3 Changed on 03/11/2014 at 08:58:46 PM by trev

  • Description modified (diff)

comment:4 Changed on 03/11/2014 at 09:11:12 PM by trev

  • Cc trev added

There is a review for this with some discussion:

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 - 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 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.

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

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:

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.

Last edited on 05/18/2014 at 12:46:55 PM by saroyanm

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: Changed on 06/30/2014 at 09:02:59 AM by 2nd

For me this line causes same error Very annoying.


Last edited on 06/30/2014 at 09:03:48 AM by 2nd

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 Very annoying.


Of course it does - the issue is only resolved in Firefox 32 and above. Earlier versions are still affected by bug 982561.

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from saroyanm.
Note: See TracTickets for help on using tickets.