Bug 153269 - REGRESSION (r195305): Web Inspector: WebInspector.Object can dispatch constructor-level events multiple times
Summary: REGRESSION (r195305): Web Inspector: WebInspector.Object can dispatch constru...
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-01-19 21:49 PST by Nikita Vasilyev
Modified: 2016-01-21 21:54 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.77 KB, patch)
2016-01-19 21:51 PST, Nikita Vasilyev
bburg: review-
Details | Formatted Diff | Diff
A Test (1.64 KB, patch)
2016-01-20 20:44 PST, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2016-01-21 18:25 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2016-01-19 21:49:06 PST
In http://trac.webkit.org/changeset/195305 I accidentally undone https://bugs.webkit.org/show_bug.cgi?id=150579,
which re-introduced the bug.

We should bring that object.hasOwnProperty("_listeners") check back.
Comment 1 Radar WebKit Bug Importer 2016-01-19 21:49:26 PST
<rdar://problem/24253106>
Comment 2 Nikita Vasilyev 2016-01-19 21:51:59 PST
Created attachment 269340 [details]
Patch
Comment 3 Joseph Pecoraro 2016-01-20 00:31:58 PST
I think we can write a test for this pretty easily! A test would be great!
Comment 4 Brian Burg 2016-01-20 07:05:27 PST
Comment on attachment 269340 [details]
Patch

As Joe said, this needs a test so we don't break it once again.
Comment 5 Nikita Vasilyev 2016-01-20 20:44:45 PST
Created attachment 269423 [details]
A Test

I wrote a test and I weren't able to reproduce the problem.

I looked closer into Bug 150579 and I realized I don't quite understand
why do we even use this propagation pattern:

    let constructor = this.constructor;
    while (constructor) {
        dispatch(constructor);

        if (!constructor.prototype.__proto__)
            break;

        constructor = constructor.prototype.__proto__.constructor;
    }

https://github.com/WebKit/webkit/blob/707ba5950cfde11635141f386dd497ee1be914e6/Source/WebInspectorUI/UserInterface/Base/Object.js#L170-L178

I commented it out and the only thing that I've noticed stopped working was the styles sidebar.
I'm trying to understand what is happening there.

Does anyone know why do we need this constructor propagation code?
Comment 6 Timothy Hatcher 2016-01-21 11:41:12 PST
(In reply to comment #5)
> Created attachment 269423 [details]
> A Test
> 
> I wrote a test and I weren't able to reproduce the problem.
> 
> I looked closer into Bug 150579 and I realized I don't quite understand
> why do we even use this propagation pattern:
> 
>     let constructor = this.constructor;
>     while (constructor) {
>         dispatch(constructor);
> 
>         if (!constructor.prototype.__proto__)
>             break;
> 
>         constructor = constructor.prototype.__proto__.constructor;
>     }
> 
> https://github.com/WebKit/webkit/blob/
> 707ba5950cfde11635141f386dd497ee1be914e6/Source/WebInspectorUI/UserInterface/
> Base/Object.js#L170-L178
> 
> I commented it out and the only thing that I've noticed stopped working was
> the styles sidebar.
> I'm trying to understand what is happening there.
> 
> Does anyone know why do we need this constructor propagation code?

This allows you to add a listener to a class or one of its super-classes. So:

Resource.addEventListener("foo", ...);
SourceCode.addEventListener("foo", ...);

If a Resource instance dispatches the "foo" event, it should fire any listeners registered on Resource *and* SourceCode. If a Script (subclass of SourceCode) fired "foo", only the SourceCode listeners should fire.

The hasOwnProperty check prevented the walk up the prototype chain from firing listeners on SourceCode for Script, and then firing them again for SourceCode.
Comment 7 Timothy Hatcher 2016-01-21 11:44:26 PST
Comment on attachment 269423 [details]
A Test

View in context: https://bugs.webkit.org/attachment.cgi?id=269423&action=review

> LayoutTests/inspector/unit-tests/object.html:22
> +            child.addEventListener(eventName, function() {

The listener should be on Parent, not an instance, and not Child to catch the bug.
Comment 8 Joseph Pecoraro 2016-01-21 12:02:20 PST
> Does anyone know why do we need this constructor propagation code?

A common thing we do is addEventListeners on a constructor `A` to receive all the events that happen for instances of `A`.

There are many cases where a subclass of class `A` will dispatch an event on behalf of the base class.

For example, `DOMTreeContentView is a subclass of `ContentView` but its instances dispatches the `ContentView.Event.SelectionPathComponentsDidChange` event. In fact that event is almost always dispatched by subclasses:

> Views/DOMTreeContentView.js
> 369:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);
> 386:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);

Listeners, like `ContentBrowser` want to handle any `ContentView.Event.SelectionPathComponentsDidChange` for any `ContentView`, so it registers on the `ContentView` constructor:

> Views/ContentBrowser.js
> 71:        WebInspector.ContentView.addEventListener(WebInspector.ContentView.Event.SelectionPathComponentsDidChange, this._contentViewSelectionPathComponentDidChange, this);

So when DOMTreeContentView dispatches the event it will be dispatched at multiple points:

    this._listeners (instance)
    this.constructor._listeners (WebInspector.DOMTreeContentView)
    this.constructor.prototype.__proto__._listeners (WebInspector.ContentView)
    ... (WebInspector.View)
    ... (WebInspectorObject)
    
I suspect removing this could would mean breaking selection path component updates all over the place. Does it?

---

Fun fact. I searched our code for examples of where we register an event on a constructor `WebInspector.Foo` where the event name is not from class Foo (e.g. not `WebInspector.Foo.Event`). I only found one example:

> Controllers/CSSStyleManager.js
> 37:        WebInspector.Resource.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._resourceContentDidChange, this);

So, for any Resource instance that dispatches a SourceCode.Event.ContentDidChange event (a superclass event), the cssStyleManager will handle it. That kind of makes sense!
Comment 9 Nikita Vasilyev 2016-01-21 18:25:36 PST
Created attachment 269531 [details]
Patch

Tim and Joe:
Thanks for the explanation. I hadn't seen event listeners being used on classes directly; it threw me off.

I was able to reproduce the bug. The test now ensures that it's fixed.
Comment 10 Nikita Vasilyev 2016-01-21 18:36:56 PST
(In reply to comment #8)
> > Does anyone know why do we need this constructor propagation code?
> 
> A common thing we do is addEventListeners on a constructor `A` to receive
> all the events that happen for instances of `A`.
> 
> There are many cases where a subclass of class `A` will dispatch an event on
> behalf of the base class.
> 
> For example, `DOMTreeContentView is a subclass of `ContentView` but its
> instances dispatches the
> `ContentView.Event.SelectionPathComponentsDidChange` event. In fact that
> event is almost always dispatched by subclasses:
> 
> > Views/DOMTreeContentView.js
> > 369:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);
> > 386:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);
> 
> Listeners, like `ContentBrowser` want to handle any
> `ContentView.Event.SelectionPathComponentsDidChange` for any `ContentView`,
> so it registers on the `ContentView` constructor:
> 
> > Views/ContentBrowser.js
> > 71:        WebInspector.ContentView.addEventListener(WebInspector.ContentView.Event.SelectionPathComponentsDidChange, this._contentViewSelectionPathComponentDidChange, this);
> 
> So when DOMTreeContentView dispatches the event it will be dispatched at
> multiple points:
> 
>     this._listeners (instance)
>     this.constructor._listeners (WebInspector.DOMTreeContentView)
>     this.constructor.prototype.__proto__._listeners
> (WebInspector.ContentView)
>     ... (WebInspector.View)
>     ... (WebInspectorObject)
>     
> I suspect removing this could would mean breaking selection path component
> updates all over the place. Does it?

Yes, the navigation bar is empty for the Elements tab.
Seems to work well for Resources/Debugger though https://cloudup.com/cBgvs4rBVS3.
Comment 11 WebKit Commit Bot 2016-01-21 21:54:42 PST
Comment on attachment 269531 [details]
Patch

Clearing flags on attachment: 269531

Committed r195442: <http://trac.webkit.org/changeset/195442>
Comment 12 WebKit Commit Bot 2016-01-21 21:54:46 PST
All reviewed patches have been landed.  Closing bug.