Bug 1340987 - WIP Crash fix draft
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Mon, 05 Jun 2017 09:23:08 +0800
changeset 588758 e9c36864c55e71ee7f83c6e46bec8b4530795d0b
parent 585049 6aa0152440a0bfb029cd972711f439dfd663cad6
child 631676 7f752d22bf361a12e858e20a1af36d572b0f32e4
push id62152
push usermozilla@noorenberghe.ca
push dateMon, 05 Jun 2017 01:24:09 +0000
bugs1340987, 12802, 12803, 12831
milestone55.0a1
Bug 1340987 - WIP Crash fix I think the crash is simply because the openDialog code path assumes[1] that the name being passed to openDialog from chrome callers is of a frame that is ready to use but now that we dynamically append a frame it's no longer ready. A simple way to know if the <browser> binding is attached is to wait for the about:blank load event after appending. This seems to work fine and you can confirm that it's not just working now because of the Promise itself by switching to a Promise.resolve() instead... it will still crash. If possible we should switch `open` to an async function and await on the about:blank load inside it. I'm not sure if that will cause other test failures or not though... it seems like opening is async anyways so it's probably better that way. [1] http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/dom/base/nsGlobalWindow.cpp#12802-12803,12831 MozReview-Commit-ID: 5tZgFwcQgzD
browser/components/preferences/in-content/subdialogs.js
browser/components/preferences/in-content/tests/head.js
--- a/browser/components/preferences/in-content/subdialogs.js
+++ b/browser/components/preferences/in-content/subdialogs.js
@@ -21,25 +21,36 @@ function SubDialog({template, parentElem
   this._frame = this._overlay.querySelector(".dialogFrame");
   this._box = this._overlay.querySelector(".dialogBox");
   this._closeButton = this._overlay.querySelector(".dialogClose");
   this._titleElement = this._overlay.querySelector(".dialogTitle");
 
   this._overlay.id = `dialogOverlay-${id}`;
   this._frame.setAttribute("name", `dialogFrame-${id}`);
 
+  //this._frameCreated = Promise.resolve();
+  this._frameCreated = new Promise(resolve => {
+    // Wait for the about:blank load to know that the frame is ready to use
+    // (the <browser> binding is attached).
+    this._frame.addEventListener("load", () => {
+      console.log(`load event for dialogFrame-${id}`); // TODO
+      resolve();
+    });
+  });
+
   parentElement.appendChild(this._overlay);
   this._overlay.hidden = false;
 }
 
 SubDialog.prototype = {
   _closingCallback: null,
   _closingEvent: null,
   _isClosing: false,
   _frame: null,
+  _frameCreated: null,
   _overlay: null,
   _box: null,
   _openedURL: null,
   _injectedStyleSheets: [
     "chrome://browser/skin/preferences/preferences.css",
     "chrome://global/skin/in-content/common.css",
     "chrome://browser/skin/preferences/in-content/preferences.css",
     "chrome://browser/skin/preferences/in-content/dialog.css",
@@ -61,44 +72,49 @@ SubDialog.prototype = {
       "xml-stylesheet",
       'href="' + aStylesheetURL + '" type="text/css"'
     );
     this._frame.contentDocument.insertBefore(contentStylesheet,
                                              this._frame.contentDocument.documentElement);
   },
 
   open(aURL, aFeatures = null, aParams = null, aClosingCallback = null) {
+    this._frameCreated.then(() => {
     // If we're open on some (other) URL or we're closing, open when closing has finished.
     if (this._openedURL || this._isClosing) {
       if (!this._isClosing) {
         this.close();
       }
       let args = Array.from(arguments);
       this._closingPromise.then(() => {
         this.open.apply(this, args);
       });
       return;
     }
     this._addDialogEventListeners();
 
     let features = (aFeatures ? aFeatures + "," : "") + "resizable,dialog=no,centerscreen";
+    let overlay = document.getElementById(`dialogOverlay-${this._id}`);
+    let b = overlay.querySelector(".dialogFrame");
+    console.log("foobar", this._id, overlay, b, b.contentDocument); // TODO
     let dialog = window.openDialog(aURL, `dialogFrame-${this._id}`, features, aParams);
     if (aClosingCallback) {
       this._closingCallback = aClosingCallback.bind(dialog);
     }
 
     this._closingEvent = null;
     this._isClosing = false;
     this._openedURL = aURL;
 
     features = features.replace(/,/g, "&");
     let featureParams = new URLSearchParams(features.toLowerCase());
     this._box.setAttribute("resizable", featureParams.has("resizable") &&
                                         featureParams.get("resizable") != "no" &&
                                         featureParams.get("resizable") != "0");
+    });
   },
 
   close(aEvent = null) {
     if (this._isClosing) {
       return;
     }
     this._isClosing = true;
     this._closingPromise = new Promise(resolve => {
--- a/browser/components/preferences/in-content/tests/head.js
+++ b/browser/components/preferences/in-content/tests/head.js
@@ -34,22 +34,17 @@ function open_preferences(aCallback) {
   let newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);
   newTabBrowser.addEventListener("Initialized", function() {
     aCallback(gBrowser.contentWindow);
   }, {capture: true, once: true});
 }
 
 function openAndLoadSubDialog(aURL, aFeatures = null, aParams = null, aClosingCallback = null) {
   let promise = promiseLoadSubDialog(aURL);
-  // XXX: Bug 1340987 introduced a change to enable multiple subdialogs, but it
-  //      also causes a timing related crash here. Wrapping it within executeSoon
-  //      is a workaround. Detail of the crash is documented in the bug above.
-  executeSoon(() => {
-    content.gSubDialog.open(aURL, aFeatures, aParams, aClosingCallback);
-  });
+  content.gSubDialog.open(aURL, aFeatures, aParams, aClosingCallback);
   return promise;
 }
 
 function promiseLoadSubDialog(aURL) {
   return new Promise((resolve, reject) => {
     content.gSubDialog._dialogStack.addEventListener("dialogopen", function dialogopen(aEvent) {
       if (aEvent.detail.dialog._frame.contentWindow.location == "about:blank")
         return;