Closed Bug 1198233 Opened 9 years ago Closed 6 years ago

abuse of bind(this) with add/removeEventListener in urlbarbindings.xml

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: Gijs, Unassigned)

References

Details

+++ This bug was initially created as a clone of Bug #1185893 +++

In the cloned bug we tried to fix urlbarbindings.xml, which currently looks like this:

3104             // HACK: The description element doesn't wrap correctly in panels,
3105             // thus set a width on it, based on the available space, before
3106             // setting its textContent.  Then set its height as well, to
3107             // fix wrong height calculation on Linux (bug 659578).
3108             this._panel.addEventListener("popupshown", function panelShown() {
3109               this._panel.removeEventListener("popupshown", panelShown, true);
3110               // Previous popupShown events may close the panel or change
3111               // its contents, so ensure this is still valid.
3112               if (this._panel.state != "open" || !this._notificationType)
3113                 return;
3114               this._promomessage.width = this._promomessage.getBoundingClientRect().width;
3115               this._promomessage.firstChild.textContent = this._notificationMessage;
3116               this._promomessage.height = this._promomessage.getBoundingClientRect().height;
3117             }.bind(this), true);


The removeEventListener call here will never do anything, so we'll run increasing numbers of this listener until we stop showing the promo box.

The try push for fixing that produced a significant increase in timeouts in  toolkit/components/passwordmgr/test/test_notifications_popup.html . I presume this has to do with the sync promo being shown because of run-by-dir and the test opening the password PopupNotification.

Paolo, do you have time to take a look at what is going on here in more detail?
Flags: needinfo?(paolo.mozmail)
Blocks: 1185893
No longer depends on: 1185893
(In reply to :Gijs Kruitbosch from comment #0)
> The try push for fixing that produced a significant increase in timeouts in 
> toolkit/components/passwordmgr/test/test_notifications_popup.html
> 
> Paolo, do you have time to take a look at what is going on here in more
> detail?

Will try to look into this if Matt doesn't beat me at it. Is this blocking anything?
Flags: needinfo?(MattN+bmo)
(In reply to :Paolo Amadini from comment #2)
> (In reply to :Gijs Kruitbosch from comment #0)
> > The try push for fixing that produced a significant increase in timeouts in 
> > toolkit/components/passwordmgr/test/test_notifications_popup.html
> > 
> > Paolo, do you have time to take a look at what is going on here in more
> > detail?
> 
> Will try to look into this if Matt doesn't beat me at it. Is this blocking
> anything?

No, it just doesn't feel great to think we're shipping code that doesn't do what it says on the tin, while fixing that breaks our automated tests...
Took a look at the failure and concluded that this requires in-depth debugging.

This is the last test case we run before the timeout:

mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_notifications_popup.html?force=1#118

And this is the last log line we get:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/notification_common.js#64

Just like you, I don't see just by looking at the code how the timeout can be related to the changes to fix the event listener removal.

This whole test should be converted to the browser-chrome framework and made compatible with e10s eventually...
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(MattN+bmo)
I cannot find anymore the code in comment 0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.