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)
Firefox
Address Bar
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)
Reporter | ||
Comment 1•9 years ago
|
||
Try push for context: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab253bb73ae3
Reporter | ||
Updated•9 years ago
|
Comment 2•9 years ago
|
||
(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)
Reporter | ||
Comment 3•9 years ago
|
||
(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...
Comment 4•9 years ago
|
||
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)
Comment 5•6 years ago
|
||
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.
Description
•