Bug 173185 - Web Inspector: REGRESSION(r217749): Event listeners are removed even if they haven't been added
Summary: Web Inspector: REGRESSION(r217749): Event listeners are removed even if they ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-09 15:01 PDT by Devin Rousso
Modified: 2017-06-10 00:13 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.20 KB, patch)
2017-06-09 15:04 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (3.46 KB, patch)
2017-06-09 21:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 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.
Comment 1 Devin Rousso 2017-06-09 15:04:59 PDT
Created attachment 312496 [details]
Patch
Comment 2 Joseph Pecoraro 2017-06-09 18:28:47 PDT
Does this trigger benign assertions or something?
Comment 3 Joseph Pecoraro 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.
Comment 4 Joseph Pecoraro 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).
Comment 5 Devin Rousso 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.");
```
Comment 6 WebKit Commit Bot 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.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2017-06-10 00:13:27 PDT
All reviewed patches have been landed.  Closing bug.