Changeset 172396 in webkit


Ignore:
Timestamp:
Aug 11, 2014 11:10:25 AM (10 years ago)
Author:
Brian Burg
Message:

Web Inspector: Add a helper to avoid leaking single-fire event listeners in Promise chains
​https://bugs.webkit.org/show_bug.cgi?id=135772

Reviewed by Timothy Hatcher.

Source/WebInspectorUI:

A common pattern when working with promise chains is to convert an event
handler into a promise by using a single-fire event listener with the
resolve continuation as the callback. This is fine if the event fires;
if it doesn't fire, then the event emitter permanently keeps a reference to the
this-object and the callback.

This patch adds EventListener, a proxy object for events that can be manipulated
from multiple promise callback functions. If a promise is rejected, the catch
block can disconnect any event listeners set up earlier in the promise chain.

This patch also reimplements EventListenerSet to use multiple EventListeners,
since they share the same logic to uniformly handle Inspector and DOM events.

Test: inspector/event-listener.html
Test: inspector/event-listener-set.html

  • UserInterface/Base/EventListener.js: Added.

(WebInspector.EventListener):
(WebInspector.EventListener.prototype.this._callback):
(WebInspector.EventListener.prototype.connect):
(WebInspector.EventListener.prototype.disconnect):

  • UserInterface/Base/EventListenerSet.js: Update license block.

(WebInspector.EventListenerSet.prototype.register):
(WebInspector.EventListenerSet.prototype.install):
(WebInspector.EventListenerSet.prototype.uninstall):

  • UserInterface/Main.html: Include EventListener.
  • UserInterface/Test.html: Include EventListener and EventListenerSet.

LayoutTests:

  • inspector/event-listener-expected.txt: Added.
  • inspector/event-listener-set-expected.txt: Added.
  • inspector/event-listener-set.html: Added.
  • inspector/event-listener.html: Added.
Location:
trunk
Files:
5 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r172381 r172396  
     12014-08-11  Brian J. Burg  <burg@cs.washington.edu>
     2
     3        Web Inspector: Add a helper to avoid leaking single-fire event listeners in Promise chains
     4        https://bugs.webkit.org/show_bug.cgi?id=135772
     5
     6        Reviewed by Timothy Hatcher.
     7
     8        * inspector/event-listener-expected.txt: Added.
     9        * inspector/event-listener-set-expected.txt: Added.
     10        * inspector/event-listener-set.html: Added.
     11        * inspector/event-listener.html: Added.
     12
    1132014-08-10  Oliver Hunt  <oliver@apple.com>
    214

  • trunk/Source/WebInspectorUI/ChangeLog

    r172379 r172396  
     12014-08-11  Brian J. Burg  <burg@cs.washington.edu>
     2
     3        Web Inspector: Add a helper to avoid leaking single-fire event listeners in Promise chains
     4        https://bugs.webkit.org/show_bug.cgi?id=135772
     5
     6        Reviewed by Timothy Hatcher.
     7
     8        A common pattern when working with promise chains is to convert an event
     9        handler into a promise by using a single-fire event listener with the
     10        resolve continuation as the callback. This is fine if the event fires;
     11        if it doesn't fire, then the event emitter permanently keeps a reference to the
     12        this-object and the callback.
     13
     14        This patch adds EventListener, a proxy object for events that can be manipulated
     15        from multiple promise callback functions. If a promise is rejected, the catch
     16        block can disconnect any event listeners set up earlier in the promise chain.
     17
     18        This patch also reimplements EventListenerSet to use multiple EventListeners,
     19        since they share the same logic to uniformly handle Inspector and DOM events.
     20
     21        Test: inspector/event-listener.html
     22        Test: inspector/event-listener-set.html
     23
     24        * UserInterface/Base/EventListener.js: Added.
     25        (WebInspector.EventListener):
     26        (WebInspector.EventListener.prototype.this._callback):
     27        (WebInspector.EventListener.prototype.connect):
     28        (WebInspector.EventListener.prototype.disconnect):
     29        * UserInterface/Base/EventListenerSet.js: Update license block.
     30        (WebInspector.EventListenerSet.prototype.register):
     31        (WebInspector.EventListenerSet.prototype.install):
     32        (WebInspector.EventListenerSet.prototype.uninstall):
     33        * UserInterface/Main.html: Include EventListener.
     34        * UserInterface/Test.html: Include EventListener and EventListenerSet.
     35
    1362014-08-10  Timothy Hatcher  <timothy@apple.com>
    237

  • trunk/Source/WebInspectorUI/UserInterface/Base/EventListenerSet.js

    r164543 r172396  
    11/*
    2  * Copyright (C) 2013 University of Washington. All rights reserved.
     2 * Copyright (C) 2013, 2014 University of Washington. All rights reserved.
    33 * Copyright (C) 2014 Apple Inc. All rights reserved.
    44 *
    … …  
    1212 *    documentation and/or other materials provided with the distribution.
    1313 *
    14  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
    15  * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
    16  * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
    17  * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
    18  * HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
    19  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
    20  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
    21  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
    22  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
     14 * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
     15 * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
     16 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
     17 * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
     18 * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
     19 * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
     20 * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
     21 * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
     22 * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
    2323 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
    2424 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
    … …  
    3939
    4040WebInspector.EventListenerSet.prototype = {
    41     register: function(emitter, type, listener, thisObject, useCapture)
     41    register: function(emitter, type, callback, thisObject, usesCapture)
    4242    {
    43         console.assert(listener, "Missing listener for event: " + type);
    44         console.assert(emitter, "Missing event emitter for event: " + type);
    45         console.assert(emitter instanceof WebInspector.Object || emitter instanceof Node || (typeof emitter.addEventListener === "function"), "Event emitter", emitter, " (type:" + type + ") does not implement Node or WebInspector.Object!");
     43        console.assert(callback, "Missing callback for event: " + type);
     44        console.assert(type, "Tried to register listener for unknown event: " + type);
     45        var emitterIsValid = emitter && (emitter instanceof WebInspector.Object || emitter instanceof Node || (typeof emitter.addEventListener === "function"));
     46        console.assert(emitterIsValid,  "Event emitter ", emitter, " (type:" + type + ") is null or does not implement Node or WebInspector.Object!");
    4647
    47         if (emitter instanceof Node)
    48             listener = listener.bind(thisObject || this._defaultThisObject);
     48        if (!emitterIsValid || !type || !callback)
     49            return;
    4950
    50         this._listeners.push({emitter: emitter, type: type, listener: listener, thisObject: thisObject, useCapture: useCapture});
     51        this._listeners.push({listener: new WebInspector.EventListener(thisObject || this._defaultThisObject), emitter: emitter, type: type, callback: callback, usesCapture: usesCapture});
    5152    },
    5253
    … …  
    6162    {
    6263        console.assert(!this._installed, "Already installed listener group: " + this.name);
     64        if (this._installed)
     65            return;
    6366
    6467        this._installed = true;
    6568
    66         for (var listenerData of this._listeners) {
    67             if (listenerData.emitter instanceof Node)
    68                 listenerData.emitter.addEventListener(listenerData.type, listenerData.listener, listenerData.useCapture);
    69             else
    70                 listenerData.emitter.addEventListener(listenerData.type, listenerData.listener, listenerData.thisObject || this._defaultThisObject);
    71         }
     69        for (var data of this._listeners)
     70            data.listener.connect(data.emitter, data.type, data.callback, data.usesCapture);
    7271    },
    7372
    … …  
    7574    {
    7675        console.assert(this._installed, "Trying to uninstall listener group " + this.name + ", but it isn't installed.");
     76        if (!this._installed)
     77            return;
    7778
    7879        this._installed = false;
    7980
    80         for (var listenerData of this._listeners) {
    81             if (listenerData.emitter instanceof Node)
    82                 listenerData.emitter.removeEventListener(listenerData.type, listenerData.listener, listenerData.useCapture);
    83             else
    84                 listenerData.emitter.removeEventListener(listenerData.type, listenerData.listener, listenerData.thisObject || this._defaultThisObject);
    85         }
     81        for (var data of this._listeners)
     82            data.listener.disconnect();
    8683
    87         if (unregisterListeners) {
     84        if (unregisterListeners)
    8885            this._listeners = [];
    89             delete this._defaultThisObject;
    90         }
    9186    },
    9287}

  • trunk/Source/WebInspectorUI/UserInterface/Main.html

    r171790 r172396  
    166166
    167167    <script src="Base/DOMUtilities.js"></script>
     168    <script src="Base/EventListener.js"></script>
    168169    <script src="Base/EventListenerSet.js"></script>
    169170    <script src="Base/ImageUtilities.js"></script>

  • trunk/Source/WebInspectorUI/UserInterface/Test.html

    r166040 r172396  
    3636
    3737    <script src="Base/DOMUtilities.js"></script>
     38    <script src="Base/EventListener.js"></script>
     39    <script src="Base/EventListenerSet.js"></script>
    3840    <script src="Base/URLUtilities.js"></script>
    3941    <script src="Base/Utilities.js"></script>
Note: See TracChangeset for help on using the changeset viewer.