Bug 173185

Summary: Web Inspector: REGRESSION(r217749): Event listeners are removed even if they haven't been added
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, joepeck
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=172381
Attachments:
Description Flags
Patch
joepeck: review+
Patch none

Devin Rousso
Reported 2017-06-09 15:01:31 PDT
The event listeners are only applied in the ResourceDetailsSidebarPanel after it has initialLayout(). If the panel is created without ever being shown, each time it is hidden() it will attempt to remove the event listeners. Since it has not been initialLayout(), these event listeners would not have already been added.
Attachments
Patch (3.20 KB, patch)
2017-06-09 15:04 PDT, Devin Rousso 		joepeck: review+
DetailsFormatted DiffDiff
Patch (3.46 KB, patch)
2017-06-09 21:33 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2017-06-09 15:04:59 PDT
Created attachment 312496 [details] Patch
Joseph Pecoraro
Comment 2 2017-06-09 18:28:47 PDT
Does this trigger benign assertions or something?
Joseph Pecoraro
Comment 3 2017-06-09 18:29:25 PDT
Comment on attachment 312496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312496&action=review > Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSidebarPanel.js:94 > + this._needsToRemoveResourceEventListeners = false; Can you also put something up in the constructor that initializes this member variable.
Joseph Pecoraro
Comment 4 2017-06-09 18:30:11 PDT
Comment on attachment 312496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312496&action=review >> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSidebarPanel.js:94 >> + this._needsToRemoveResourceEventListeners = false; > > Can you also put something up in the constructor that initializes this member variable. Oh this is in the constructor (implicitly).
Devin Rousso
Comment 5 2017-06-09 21:33:27 PDT
Created attachment 312558 [details] Patch (In reply to Joseph Pecoraro from comment #2) > Does this trigger benign assertions or something? Yes, it causes assertions in `removeEventListener` ``` console.assert(didDelete, "removeEventListener cannot remove " + eventType.toString() + " because it doesn't exist."); ```
WebKit Commit Bot
Comment 6 2017-06-09 21:56:59 PDT
The commit-queue encountered the following flaky tests while processing attachment 312558 [details]: transforms/hittest-translated-content-off-to-infinity-and-back.html bug 173222 (author: zalan@apple.com) imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html bug 172934 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 7 2017-06-10 00:13:25 PDT
Comment on attachment 312558 [details] Patch Clearing flags on attachment: 312558 Committed r218049: <http://trac.webkit.org/changeset/218049>
WebKit Commit Bot
Comment 8 2017-06-10 00:13:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.