Bug 1511130 - Part II, Allow UAWidgetsChild to destruct widgets even if it throws during construction. r=bgrins, a=RyanVM
authorTimothy Guan-tin Chien <timdream@gmail.com>
Thu, 13 Dec 2018 20:59:00 +0000
changeset 509094 c41c18787e43e3ce0d9008226f21810eb19c634b
parent 509093 17a1d1be96f474b846b1bdbb82786c3238407b53
child 509095 325595e085d7acfdd80ea333a9f3dba85cd3b1e6
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbgrins, RyanVM
bugs1511130
milestone65.0
Bug 1511130 - Part II, Allow UAWidgetsChild to destruct widgets even if it throws during construction. r=bgrins, a=RyanVM This patch move the actual widget construction to a onsetup method, allow UAWidgetsChild to hold the reference of the widget instance even if the actual setup (happens in the onsetup call) throws. With the reference of the widget kept, UAWidgetsChild will finally able to call its destructor later on. Depends on D13607 Differential Revision: https://phabricator.services.mozilla.com/D13608
dom/media/tests/crashtests/1505957.html
dom/media/tests/crashtests/1511130.html
dom/media/tests/crashtests/crashtests.list
toolkit/actors/UAWidgetsChild.jsm
toolkit/content/widgets/datetimebox.js
toolkit/content/widgets/docs/ua_widget.rst
toolkit/content/widgets/marquee.js
toolkit/content/widgets/pluginProblem.js
toolkit/content/widgets/videocontrols.js
--- a/dom/media/tests/crashtests/1505957.html
+++ b/dom/media/tests/crashtests/1505957.html
@@ -1,13 +1,13 @@
 <html id='a'>
 <script>
 window.onload=function() {
     b.src=document.getElementById('c').innerHTML;
-    b.setAttribute('src', 'data:audio/mpeg,');
+    b.setAttribute('src', 'video-crash.webm');
     document.documentElement.style.display='none';
     window.top.open('');
     var o = window.frames[0].document.body.childNodes[0];
     document.getElementById('d').appendChild(o.parentNode.removeChild(o));
     o = document.getElementById('a');
     var p = o.parentNode;
     o.setAttribute('id', 0)
     p.removeChild(o);
new file mode 100644
--- /dev/null
+++ b/dom/media/tests/crashtests/1511130.html
@@ -0,0 +1,12 @@
+<script>
+document.addEventListener("DOMContentLoaded", function(){
+  var o = window.frames[0].document.body.childNodes[0];
+  a.appendChild(o.parentNode.removeChild(o));
+  o = document.body;
+  o.parentNode.appendChild(o);
+  o.parentNode.appendChild(o);
+})
+</script>
+<body text=''>
+<hgroup id='a'>
+<iframe src="video-crash.webm"></iframe>
--- a/dom/media/tests/crashtests/crashtests.list
+++ b/dom/media/tests/crashtests/crashtests.list
@@ -20,8 +20,9 @@ load 1306476.html
 load 1348381.html
 load 1367930_1.html
 load 1367930_2.html
 pref(browser.link.open_newwindow,2) load 1429507_1.html # window.open() in tab doesn't work for crashtest in e10s, this opens a new window instead
 pref(browser.link.open_newwindow,2) load 1429507_2.html # window.open() in tab doesn't work for crashtest in e10s, this opens a new window instead
 load 1453030.html
 skip-if(Android) load 1490700.html # No screenshare on Android
 load 1505957.html
+load 1511130.html
--- a/toolkit/actors/UAWidgetsChild.jsm
+++ b/toolkit/actors/UAWidgetsChild.jsm
@@ -33,29 +33,33 @@ class UAWidgetsChild extends ActorChild 
   }
 
   setupOrNotifyWidget(aElement) {
     let widget = this.widgets.get(aElement);
     if (!widget) {
       this.setupWidget(aElement);
       return;
     }
-    if (typeof widget.wrappedJSObject.onattributechange == "function") {
-      widget.wrappedJSObject.onattributechange();
+    if (typeof widget.wrappedJSObject.onchange == "function") {
+      try {
+        widget.wrappedJSObject.onchange();
+      } catch (ex) {
+        Cu.reportError(ex);
+      }
     }
   }
 
   setupWidget(aElement) {
     let uri;
     let widgetName;
     switch (aElement.localName) {
       case "video":
       case "audio":
         uri = "chrome://global/content/elements/videocontrols.js";
-        widgetName = "VideoControlsPageWidget";
+        widgetName = "VideoControlsWidget";
         break;
       case "input":
         uri = "chrome://global/content/elements/datetimebox.js";
         widgetName = "DateTimeBoxWidget";
         break;
       case "embed":
       case "object":
         uri = "chrome://global/content/elements/pluginProblem.js";
@@ -67,30 +71,49 @@ class UAWidgetsChild extends ActorChild 
         break;
     }
 
     if (!uri || !widgetName) {
       return;
     }
 
     let shadowRoot = aElement.openOrClosedShadowRoot;
-    let sandbox = aElement.nodePrincipal.isSystemPrincipal ?
+    if (!shadowRoot) {
+      Cu.reportError("Getting a UAWidgetBindToTree/UAWidgetAttributeChanged event without the Shadow Root.");
+      return;
+    }
+
+    let isSystemPrincipal = aElement.nodePrincipal.isSystemPrincipal;
+    let sandbox = isSystemPrincipal ?
       Object.create(null) : Cu.getUAWidgetScope(aElement.nodePrincipal);
 
     if (!sandbox[widgetName]) {
       Services.scriptloader.loadSubScript(uri, sandbox, "UTF-8");
     }
 
     let widget = new sandbox[widgetName](shadowRoot);
     this.widgets.set(aElement, widget);
+    try {
+      if (!isSystemPrincipal) {
+        widget.wrappedJSObject.onsetup();
+      } else {
+        widget.onsetup();
+      }
+    } catch (ex) {
+      Cu.reportError(ex);
+    }
   }
 
   teardownWidget(aElement) {
     let widget = this.widgets.get(aElement);
     if (!widget) {
       return;
     }
     if (typeof widget.wrappedJSObject.destructor == "function") {
-      widget.wrappedJSObject.destructor();
+      try {
+        widget.wrappedJSObject.destructor();
+      } catch (ex) {
+        Cu.reportError(ex);
+      }
     }
     this.widgets.delete(aElement);
   }
 }
--- a/toolkit/content/widgets/datetimebox.js
+++ b/toolkit/content/widgets/datetimebox.js
@@ -12,24 +12,29 @@
  * according to the value of the "type" property.
  */
 this.DateTimeBoxWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     this.document = this.element.ownerDocument;
     this.window = this.document.defaultView;
+  }
 
+  /*
+   * Callback called by UAWidgets right after constructor.
+   */
+  onsetup() {
     this.switchImpl();
   }
 
   /*
    * Callback called by UAWidgets when the "type" property changes.
    */
-  onattributechange() {
+  onchange() {
     this.switchImpl();
   }
 
   /*
    * Actually switch the implementation.
    * - With type equal to "date", DateInputImplWidget should load.
    * - With type equal to "time", TimeInputImplWidget should load.
    * - Otherwise, nothing should load and loaded impl should be unloaded.
@@ -47,16 +52,17 @@ this.DateTimeBoxWidget = class {
       return;
     }
     if (this.impl) {
       this.impl.destructor();
       this.shadowRoot.firstChild.remove();
     }
     if (newImpl) {
       this.impl = new newImpl(this.shadowRoot);
+      this.impl.onsetup();
     } else {
       this.impl = undefined;
     }
   }
 
   destructor() {
     if (!this.impl) {
       return;
@@ -68,22 +74,24 @@ this.DateTimeBoxWidget = class {
 };
 
 this.DateTimeInputBaseImplWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     this.document = this.element.ownerDocument;
     this.window = this.document.defaultView;
+  }
 
+  onsetup() {
     this.generateContent();
 
 
     this.DEBUG = false;
-    this.mDateTimeBoxElement = shadowRoot.firstChild;
+    this.mDateTimeBoxElement = this.shadowRoot.firstChild;
     this.mInputElement = this.element;
     this.mLocales = this.window.getRegionalPrefsLocales();
 
     this.mIsRTL = false;
     let intlUtils = this.window.intlUtils;
     if (intlUtils) {
       this.mIsRTL =
         intlUtils.getLocaleInfo(this.mLocales).direction === "rtl";
@@ -623,16 +631,20 @@ this.DateTimeInputBaseImplWidget = class
       this.mInputElement.openDateTimePicker(this.getCurrentValue());
     }
   }
 };
 
 this.DateInputImplWidget = class extends DateTimeInputBaseImplWidget {
   constructor(shadowRoot) {
     super(shadowRoot);
+  }
+
+  onsetup() {
+    super.onsetup();
 
     this.mMinMonth = 1;
     this.mMaxMonth = 12;
     this.mMinDay = 1;
     this.mMaxDay = 31;
     this.mMinYear = 1;
     // Maximum year limited by ECMAScript date object range, year <= 275760.
     this.mMaxYear = 275760;
@@ -961,16 +973,20 @@ this.DateInputImplWidget = class extends
     return (this.isEmpty(year) || this.isEmpty(month) ||
             this.isEmpty(day));
   }
 };
 
 this.TimeInputImplWidget = class extends DateTimeInputBaseImplWidget {
   constructor(shadowRoot) {
     super(shadowRoot);
+  }
+
+  onsetup() {
+    super.onsetup();
 
     const kDefaultAMString = "AM";
     const kDefaultPMString = "PM";
 
     let { amString, pmString } =
       this.getStringsForLocale(this.mLocales);
 
     this.mAMIndicator = amString || kDefaultAMString;
--- a/toolkit/content/widgets/docs/ua_widget.rst
+++ b/toolkit/content/widgets/docs/ua_widget.rst
@@ -10,21 +10,23 @@ UA Widget lifecycle
 -------------------
 
 UA Widgets are generally constructed when the element is appended to the document and destroyed when the element is removed from the tree. Yet, in order to be fast, specialization was made to each of the widgets.
 
 When the element is appended to the tree, a chrome-only ``UAWidgetBindToTree`` event is dispatched and is caught by a frame script, namely UAWidgetsChild.
 
 UAWidgetsChild then grabs the sandbox for that origin (lazily creating it as needed), loads the script as needed, and initializes an instance by calling the JS constructor with a reference to the UA Widget Shadow Root created by the DOM. We will discuss the sandbox in the latter section.
 
+The ``onsetup`` method is called right after the instance is constructed. The call to constructor must not throw, or UAWidgetsChild will be confused since an instance of the widget will not be returned, but the widget is already half-initalized. If the ``onsetup`` method call throws, UAWidgetsChild will still be able to hold the reference of the widget and call the destructor later on.
+
 When the element is removed from the tree, ``UAWidgetUnbindFromTree`` is dispatched so UAWidgetsChild can destroy the widget, if it exists. If so, the UAWidgetsChild calls the ``destructor()`` method on the widget, causing the widget to destruct itself.
 
 When a UA Widget initializes, it should create its own DOM inside the passed UA Widget Shadow Root, including the ``<link>`` element necessary to load the stylesheet, add event listeners, etc. When destroyed (i.e. the destructor method is called), it should do the opposite.
 
-**Specialization**: for video controls, we do not want to do the work if the control is not needed (i.e. when the ``<video>`` or ``<audio>`` element has no "controls" attribute set), so we forgo dispatching the event from HTMLMediaElement in the BindToTree method. Instead, a ``UAWidgetAttributeChanged`` event will cause the sandbox and the widget instance to construct when the attribute is set to true. The same event is also responsible for triggering the ``onattributechange()`` method on UA Widgets if the widget is already initialized.
+**Specialization**: for video controls, we do not want to do the work if the control is not needed (i.e. when the ``<video>`` or ``<audio>`` element has no "controls" attribute set), so we forgo dispatching the event from HTMLMediaElement in the BindToTree method. Instead, another ``UAWidgetAttributeChanged`` event will cause the sandbox and the widget instance to construct when the attribute is set to true. The same event is also responsible for triggering the ``onchange()`` method on UA Widgets if the widget is already initialized.
 
 Likewise, the datetime box widget is only loaded when the ``type`` attribute of an ``<input>`` is either `date` or `time`.
 
 The specialization does not apply to the lifecycle of the UA Widget Shadow Root. It is always constructed in order to suppress children of the DOM element from the web content from receiving a layout frame.
 
 UA Widget Shadow Root
 ---------------------
 
@@ -42,11 +44,11 @@ While the closed shadow root technically
 To avoid creating reflectors before DOM insertion, the available DOM interfaces is limited. For example, instead of ``createElement()`` and ``appendChild()``, the script would have to call ``createElementAndAppendChildAt()`` available on the UA Widget Shadow Root instance, to avoid receiving a reference to the DOM element and thus triggering the creation of its reflector in the wrong scope, before the element is properly associated with the UA Widget shadow tree. To find out the differences, search for ``Func="IsChromeOrXBLOrUAWidget"`` and ``Func="IsNotUAWidget"`` in in-tree WebIDL files.
 
 Other things to watch out for
 -----------------------------
 
 As part of the implementation of the Web Platform, it is important to make sure the web-observable characteristics of the widget correctly reflect what the script on the web expects.
 
 * Do not dispatch non-spec compliant events on the UA Widget Shadow Root host element, as event listeners in web content scripts can access them.
-* The layout and the dimensions of the widget should be ready by the time the constructor returns, since they can be detectable as soon as the content script gets the reference of the host element (i.e. when ``appendChild()`` returns). In order to make this easier we load ``<link>`` elements load chrome stylesheets synchronously when inside a UA Widget Shadow DOM.
+* The layout and the dimensions of the widget should be ready by the time the constructor and ``onsetup`` returns, since they can be detectable as soon as the content script gets the reference of the host element (i.e. when ``appendChild()`` returns). In order to make this easier we load ``<link>`` elements load chrome stylesheets synchronously when inside a UA Widget Shadow DOM.
 * There shouldn't be any white-spaces nodes in the Shadow DOM, because UA Widget could be placed inside ``white-space: pre``. See bug 1502205.
 * CSP will block inline styles in the Shadow DOM. ``<link>`` is the only safe way to load styles.
--- a/toolkit/content/widgets/marquee.js
+++ b/toolkit/content/widgets/marquee.js
@@ -7,25 +7,30 @@
 /*
  * This is the class of entry. It will construct the actual implementation
  * according to the value of the "direction" property.
  */
 this.MarqueeWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
+  }
 
+  /*
+   * Callback called by UAWidgets right after constructor.
+   */
+  onsetup() {
     this.switchImpl();
   }
 
   /*
    * Callback called by UAWidgetsChild wheen the direction property
    * changes.
    */
-  onattributechange() {
+  onchange() {
     this.switchImpl();
   }
 
   switchImpl() {
     let newImpl;
     switch (this.element.direction) {
       case "up":
       case "down":
@@ -40,16 +45,17 @@ this.MarqueeWidget = class {
     // Skip if we are asked to load the same implementation.
     // This can happen if the property is set again w/o value change.
     if (this.impl && this.impl.constructor == newImpl) {
       return;
     }
     this.destructor();
     if (newImpl) {
       this.impl = new newImpl(this.shadowRoot);
+      this.impl.onsetup();
     }
   }
 
   destructor() {
     if (!this.impl) {
       return;
     }
     this.impl.destructor();
@@ -59,17 +65,19 @@ this.MarqueeWidget = class {
 };
 
 this.MarqueeBaseImplWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     this.document = this.element.ownerDocument;
     this.window = this.document.defaultView;
+  }
 
+  onsetup() {
     this.generateContent();
 
     // Set up state.
     this._currentDirection = this.element.direction || "left";
     this._currentLoop = this.element.loop;
     this.dirsign = 1;
     this.startAt = 0;
     this.stopAt = 0;
--- a/toolkit/content/widgets/pluginProblem.js
+++ b/toolkit/content/widgets/pluginProblem.js
@@ -8,17 +8,19 @@
 // to be loaded by UAWidgetsChild.jsm.
 
 this.PluginProblemWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     // ownerGlobal is chrome-only, not accessible to UA Widget script here.
     this.window = this.element.ownerDocument.defaultView; // eslint-disable-line mozilla/use-ownerGlobal
+  }
 
+  onsetup() {
     const parser = new this.window.DOMParser();
     let parserDoc = parser.parseFromString(`
       <!DOCTYPE bindings [
         <!ENTITY % pluginproblemDTD SYSTEM "chrome://pluginproblem/locale/pluginproblem.dtd">
         <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
         <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
         %pluginproblemDTD;
         %globalDTD;
--- a/toolkit/content/widgets/videocontrols.js
+++ b/toolkit/content/widgets/videocontrols.js
@@ -1,86 +1,94 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
-// This is a page widget. It runs in per-origin UA widget scope,
+// This is a UA widget. It runs in per-origin UA widget scope,
 // to be loaded by UAWidgetsChild.jsm.
 
 /*
  * This is the class of entry. It will construct the actual implementation
  * according to the value of the "controls" property.
  */
-this.VideoControlsPageWidget = class {
+this.VideoControlsWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     this.document = this.element.ownerDocument;
     this.window = this.document.defaultView;
 
     this.isMobile = this.window.navigator.appVersion.includes("Android");
+  }
 
+  /*
+   * Callback called by UAWidgets right after constructor.
+   */
+  onsetup() {
     this.switchImpl();
   }
 
   /*
    * Callback called by UAWidgets when the "controls" property changes.
    */
-  onattributechange() {
+  onchange() {
     this.switchImpl();
   }
 
   /*
    * Actually switch the implementation.
-   * - With "controls" set, the VideoControlsImplPageWidget controls should load.
-   * - Without it, on mobile, the NoControlsImplPageWidget should load, so
+   * - With "controls" set, the VideoControlsImplWidget controls should load.
+   * - Without it, on mobile, the NoControlsImplWidget should load, so
    *   the user could see the click-to-play button when the video/audio is blocked.
    */
   switchImpl() {
     let newImpl;
     if (this.element.controls) {
-      newImpl = VideoControlsImplPageWidget;
+      newImpl = VideoControlsImplWidget;
     } else if (this.isMobile) {
-      newImpl = NoControlsImplPageWidget;
+      newImpl = NoControlsImplWidget;
     }
     // Skip if we are asked to load the same implementation.
     // This can happen if the property is set again w/o value change.
     if (this.impl && this.impl.constructor == newImpl) {
       return;
     }
     if (this.impl) {
       this.impl.destructor();
       this.shadowRoot.firstChild.remove();
     }
     if (newImpl) {
       this.impl = new newImpl(this.shadowRoot);
+      this.impl.onsetup();
     } else {
       this.impl = undefined;
     }
   }
 
   destructor() {
     if (!this.impl) {
       return;
     }
     this.impl.destructor();
     this.shadowRoot.firstChild.remove();
     delete this.impl;
   }
 };
 
-this.VideoControlsImplPageWidget = class {
+this.VideoControlsImplWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     this.document = this.element.ownerDocument;
     this.window = this.document.defaultView;
+  }
 
+  onsetup() {
     this.generateContent();
 
     this.Utils = {
       debug: false,
       video: null,
       videocontrols: null,
       controlBar: null,
       playButton: null,
@@ -2284,23 +2292,25 @@ this.VideoControlsImplPageWidget = class
     this.shadowRoot.firstChild.addEventListener("mousemove", event => {
       if (!this.Utils.isTouchControls) {
         this.Utils.onMouseMove(event);
       }
     });
   }
 };
 
-this.NoControlsImplPageWidget = class {
+this.NoControlsImplWidget = class {
   constructor(shadowRoot) {
     this.shadowRoot = shadowRoot;
     this.element = shadowRoot.host;
     this.document = this.element.ownerDocument;
     this.window = this.document.defaultView;
+  }
 
+  onsetup() {
     this.generateContent();
 
     this.Utils = {
       videoEvents: ["play",
                     "playing",
                     "MozNoControlsBlockedVideo"],
       terminate() {
         for (let event of this.videoEvents) {