Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#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):

http://codereview.adblockplus.org/5121479175831552/

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

Change History (22)

comment:1 Changed 6 years ago by philll

  • Description modified (diff)

comment:2 Changed 6 years ago by trev

  • Description modified (diff)
  • Summary changed from Fix deprecated warning in Firefox Nightly to Get rid of deprecated getUserData() methid

comment:3 Changed 6 years ago by trev

  • Description modified (diff)

comment:4 Changed 6 years ago by trev

  • Cc trev added

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.

comment:5 Changed 6 years ago by trev

  • Summary changed from Get rid of deprecated getUserData() methid to Get rid of deprecated getUserData() method

comment:7 Changed 6 years ago by trev

  • Keywords externaldependency added

comment:8 Changed 6 years ago by philll

  • Owner set to saroyanm
  • Status changed from new to assigned

comment:9 Changed 6 years ago by fhd

  • Priority changed from Unknown to P4
  • Reporter changed from philll to saroyanm

comment:10 Changed 6 years ago 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 6 years ago 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 6 years ago by tschuster

Interesting, I will take a look at the Mozila bug, maybe I can fix it.

comment:13 Changed 6 years ago 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 5 years ago by philll

  • Status changed from assigned to new

The assigned state will be dropped by #403

comment:15 Changed 5 years ago by trev

  • Owner saroyanm deleted

Unassigning - I don't think this is currently being worked on.

comment:16 Changed 5 years ago 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 5 years ago by saroyanm (previous) (diff)

comment:17 Changed 5 years ago by saroyanm

  • Owner set to saroyanm

comment:18 Changed 5 years ago by saroyanm

  • Review URL(s) modified (diff)

comment:19 Changed 5 years ago by saroyanm

  • Resolution set to fixed
  • Status changed from new to closed

comment:20 Changed 5 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Ready set

comment:21 follow-up: Changed 5 years ago 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

Last edited 5 years ago by 2nd (previous) (diff)

comment:22 in reply to: ↑ 21 Changed 5 years ago 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.

Note: See TracTickets for help on using tickets.