Improve subdialog.js to support multiple dialogs

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Preferences
RESOLVED FIXED
4 months ago
21 days ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M2] ETA:612)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

Comment hidden (empty)
Hey Scott, in the future please don't leave the description blank as it makes it harder to find bugs and adds context for others who find it and may not understand what it's about. Thanks
Component: Form Manager → Preferences
Product: Toolkit → Firefox
Comment hidden (mozreview-request)
(Assignee)

Comment 3

3 months ago
I've changed the `gSubDialog` function so that it acts like a SubDialog manager. When `gSubDialog.open` is called, it creates a new SubDialog instance, and add/remove listeners accordingly. I also removed some promise code that handles closing timing. I don't think it's necessary anymore because closing happens synchronously now when the entire SubDialog node is removed (rather than unloaded).

The test cases don't work yet because I've changed the DOM structure. I'll update them and upload the patch ASAP.
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
The patch has been updated to produce a more sensible diff. Updated `browser_subdialogs.js` tests, but there are still 3 spots that are failing. Two of which are related to the page back function (which closes the dialog). I'm not sure what I did but it appears to be a focus issue.

I just noticed that there are dialog related tests in head.js and other files which I will also update.
(Assignee)

Updated

3 months ago
Blocks: 1352307
Comment on attachment 8852059 [details]
Bug 1340987 - (Part 1) Use arrow functions in gSubDialog.

https://reviewboard.mozilla.org/r/124284/#review129202

::: browser/components/preferences/in-content/preferences.xul:203
(Diff revision 2)
>    </hbox>
> -
> +    <stack id="dialogStack"/>
> -    <vbox id="dialogOverlay" align="center" pack="center">
> -      <groupbox id="dialogBox"

Note that you will need to make the change to in-content-old too. Same for subdialos.js I think though maybe we can unfork that file?

::: browser/components/preferences/in-content/subdialogs.js:58
(Diff revision 2)
> +    title.className = "dialogTitle";
> +    title.setAttribute("flex", 1);
> +
> +    let closeButton = document.createElementNS(XUL_NS, "button");
> +    closeButton.className = "dialogClose close-icon";
> +    // closeButton.setAttirbute("aria-label");

Instead of switching this all to be created programmatically did you consider having the "template" in the document (e.g. adjacent to the stack) and cloning it via cloneNode for each opened dialog? Similar to the idea of using <template> (though this is XUL)? That would allow this DTD string to be resolved by the parser properly.

::: browser/components/preferences/in-content/subdialogs.js:98
(Diff revision 2)
>      );
>      this._frame.contentDocument.insertBefore(contentStylesheet,
>                                               this._frame.contentDocument.documentElement);
>    },
>  
> -  open(aURL, aFeatures = null, aParams = null, aClosingCallback = null) {
> +  open(aURL, aFeatures, aParams, aClosingCallback) {

What is the reason for removing the argument defaults values? They were there to indicate which arguments are optional. Are you wanting them to always be explicitly passed? I realize that real consumers will use the global gSubDialog.open.

::: browser/components/preferences/in-content/subdialogs.js:117
(Diff revision 2)
>    close(aEvent = null) {
> -    if (this._isClosing) {
> -      return;
> -    }

Can you please try to separate this patch into separate commits since it's hard to review as-is:
* Switch to arrow functions in a few places which means changing to `this` inside
* Switching to a stack and using the SubDialog prototype.
* All of the code related to removing `unload`/`_isClosing`

::: browser/components/preferences/in-content/subdialogs.js:433
(Diff revision 2)
>                   .chromeEventHandler;
>    },
>  };
> +
> +var gSubDialog = {
> +  _dialogs: [],

Could you add a JSDoc comment here about the order of the array.
Attachment #8852059 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

3 months ago
mozreview-review-reply
Comment on attachment 8852059 [details]
Bug 1340987 - (Part 1) Use arrow functions in gSubDialog.

https://reviewboard.mozilla.org/r/124284/#review129202

> Instead of switching this all to be created programmatically did you consider having the "template" in the document (e.g. adjacent to the stack) and cloning it via cloneNode for each opened dialog? Similar to the idea of using <template> (though this is XUL)? That would allow this DTD string to be resolved by the parser properly.

Ok I'm using cloneNode on the "template" for creating new dialogs now :)

> Can you please try to separate this patch into separate commits since it's hard to review as-is:
> * Switch to arrow functions in a few places which means changing to `this` inside
> * Switching to a stack and using the SubDialog prototype.
> * All of the code related to removing `unload`/`_isClosing`

I have split up the patch into different parts. It's much more readable now!
(Assignee)

Comment 13

3 months ago
Sorry about the mess on the initial patch. Hope it's more readable now :)

I've commented out a few test cases because I couldn't get it working still. It's about the fact that we should be able to use goBack to cancel/close a subdialog:
http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/browser/components/preferences/in-content/tests/browser_subdialogs.js#172-188
Flags: needinfo?(MattN+bmo)

Updated

3 months ago
Whiteboard: [form autofill:M1] → [form autofill:M2]
(Assignee)

Comment 14

3 months ago
Hi Matt, I'm trying a different approach that will possibly reduce the amount of code change and resolve the goBack issue previously mentioned. So if you haven't got around reviewing this it's okay. I'll update the patch again soon.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8857073 - Attachment is obsolete: true
Comment on attachment 8852059 [details]
Bug 1340987 - (Part 1) Use arrow functions in gSubDialog.

https://reviewboard.mozilla.org/r/124284/#review132744
Attachment #8852059 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review132802

::: browser/components/preferences/in-content/preferences.xul:188
(Diff revision 2)
>        </prefpane>
>      </vbox>
>    </hbox>
>  
> -    <vbox id="dialogOverlay" align="center" pack="center">
> -      <groupbox id="dialogBox"
> +  <stack id="dialogStack"/>
> +  <vbox id="dialogTemplate" class="dialogOverlay" align="center" pack="center" overlay="overlay">

I think the @overlay attribute should have a more semantic meaning because currently the selector for it doesn't have much semantic meaning. Is it really indicating that it's topmost? If so, use topmost="true" (XUL attributes use booleans).

::: browser/components/preferences/in-content/subdialogs.js:126
(Diff revision 2)
>      this._box.removeAttribute("width");
>      this._box.removeAttribute("height");
>      this._box.style.removeProperty("min-height");
>      this._box.style.removeProperty("min-width");
>  
> +    this._root.dispatchEvent(new CustomEvent("dialogclose"));

It seems to me like this should be dispatched on `_overlay` so that the event target is more useful and you can use instead of assume that the close is always for the topmost dialog.

::: browser/components/preferences/in-content/subdialogs.js:466
(Diff revision 2)
> +   * @type {Array}
> +   */
> +  _dialogs: [],
> +  _dialogStack: null,
> +  _dialogTemplate: null,
> +  _newDialog: null,

I guess this should be deleted since it's now a getter?

::: browser/components/preferences/in-content/subdialogs.js:467
(Diff revision 2)
> +  get _newDialog() {
> +    return this._dialogs.length > 0 ? this._dialogs[this._dialogs.length - 1] : undefined;
> +  },
> +  get _topDialog() {
> +    return this._dialogs.length > 1 ? this._dialogs[this._dialogs.length - 2] : undefined;
> +  },
> +  get _lowerDialog() {

The distinction between these three getters isn't very clear, different names would help and if that's not enough then JSDoc comments may.

::: browser/components/preferences/in-content/subdialogs.js:473
(Diff revision 2)
> +  get _lowerDialog() {
> +    return this._dialogs.length > 2 ? this._dialogs[this._dialogs.length - 3] : undefined;
> +  },

This feels a bit ununsual to me. `_lowerDialog` is always the third from the top?

::: browser/components/preferences/in-content/subdialogs.js:478
(Diff revision 2)
> +  get _lowerDialog() {
> +    return this._dialogs.length > 2 ? this._dialogs[this._dialogs.length - 3] : undefined;
> +  },
> +  init() {
> +    this._dialogStack = document.getElementById("dialogStack");
> +    this._dialogTemplate = document.querySelector(".dialogOverlay");

Why not `getElementById("dialogTemplate")`? It seems more clear and less fragile.

::: browser/components/preferences/in-content/subdialogs.js:479
(Diff revision 2)
> +    this._dialogs.push(new SubDialog({template: this._dialogTemplate,
> +                                      root: this._dialogStack,
> +                                      id: this._dialogs.length}));

Why are we pushing one at `init` time and using is as `_newDialog`? If there's a good reason it should be explained since the natural way would be to construct the subdialog just before calling its .open method

::: browser/components/preferences/in-content/subdialogs.js:527
(Diff revision 2)
> +  _onDialogClose() {
> +    this._newDialog._overlay.remove();
> +    this._dialogs.pop();
> +    if (this._topDialog) {

Assuming top is closed

::: browser/components/preferences/in-content/subdialogs.js:527
(Diff revision 2)
> +  _onDialogClose() {
> +    this._newDialog._overlay.remove();
> +    this._dialogs.pop();

So this is where we actually remove the dialog element? Why don't we have to wait until the closing promise is resolved? The `_newDialog` name is confusing to me here. Should `_newDialog` be `_topMostDialog`?

It seems to me like most of the SubDialog.prototype.close code can be removed, since we don't have to worry about re-using anymore, right?

::: browser/components/preferences/in-content/subdialogs.js:540
(Diff revision 2)
> +  _addParentEventListeners() {
> +    this._dialogStack.addEventListener("dialogopen", this);
> +    this._dialogStack.addEventListener("dialogclose", this);
> +  },

`_ensureStackEventListeners` is how I've seen code communicate that calling it multiple times won't cause problems. Note that I also replaced "Parent" with "Stack" since I think it's more clear.

::: browser/components/preferences/in-content/subdialogs.js:545
(Diff revision 2)
> +  _addParentEventListeners() {
> +    this._dialogStack.addEventListener("dialogopen", this);
> +    this._dialogStack.addEventListener("dialogclose", this);
> +  },
> +
> +  _removeParentEventListeners() {

`_removeStackEventListeners`

::: browser/themes/shared/incontentprefs/preferences.inc.css:284
(Diff revision 2)
> -#dialogOverlay {
> -  background-color: rgba(0,0,0,0.5);
> +#dialogStack,
> +#dialogTemplate,

Why not display:none for #dialogTemplate or #dialogStack? display:none allows layout to avoid frame construction. You can use the @hidden=true (XUL using boolean attributes) instead of using styles.
Comment on attachment 8857071 [details]
Bug 1340987 - (Part 3) Fix browser-chrome-mochitest for gSubDialog.

https://reviewboard.mozilla.org/r/128968/#review132820

::: browser/components/preferences/in-content/tests/browser_subdialogs.js:53
(Diff revision 2)
>        "No stylesheets that were expected are missing");
>      return result;
>    });
>  }
>  
>  function* close_subdialog_and_test_generic_end_state(browser, closingFn, closingButton, acceptCount, options) {

Can you add 2 checks in this method:
1) that gSubDialog's array of dialogs decreases by 1
2) that the count of children of the stack decreases by 1
Attachment #8857071 - Flags: review+
Don't forget to either unfork subdialogs.js or make your same changes to both before landing…
Flags: needinfo?(MattN+bmo)
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review132822

::: browser/components/preferences/in-content/tests/head.js:48
(Diff revision 2)
> -    content.gSubDialog._frame.addEventListener("load", function load(aEvent) {
> +    content.gSubDialog._newDialog._frame.addEventListener("load", function load(aEvent) {
>        if (aEvent.target.contentWindow.location == "about:blank")
>          return;
> -      content.gSubDialog._frame.removeEventListener("load", load);
> +      content.gSubDialog._newDialog._frame.removeEventListener("load", load);
>  
> -      is(content.gSubDialog._frame.contentWindow.location.toString(), aURL,
> +      is(content.gSubDialog._topDialog._frame.contentWindow.location.toString(), aURL,

This is a case where the difference between `_newDialog` and `_topDialog` are important but it's not immediately clear to me that this is correct partly due to naming.

Can you explain this to me if you don't end up cleaning this up based on part 2 suggestions?
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review132802

> Assuming top is closed

Sorry, this note was for myself. I meant to say that I think we should somehow handle close being called on a dialog which isn't top-most. The simplest would be to through an error and don't close it maybe using preventDefault?
(Assignee)

Comment 24

2 months ago
mozreview-review-reply
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review132802

> The distinction between these three getters isn't very clear, different names would help and if that's not enough then JSDoc comments may.

I have removed the _newDialog getter and renamed it to _preloadDialog. The _lowerDialog is moved into the _onDialogOpen where it's used.

> Why are we pushing one at `init` time and using is as `_newDialog`? If there's a good reason it should be explained since the natural way would be to construct the subdialog just before calling its .open method

Yeah this is a bit strange. I am changing this to only push the _preloadDialog to _dialogs only when it's opened.

> Sorry, this note was for myself. I meant to say that I think we should somehow handle close being called on a dialog which isn't top-most. The simplest would be to through an error and don't close it maybe using preventDefault?

In order to allow the dialogs below to be closed, I've a few changes:
- The reference of the closed dialog is passed into `_onDialogClose`, and check if it's the `_topDialog`.
- Since the top-most dialog might not be the one closed, `_dialogs.length` could no longer be used as a ID. `_dialogID` is used instead, and it increments whenever a dialog is created.

> So this is where we actually remove the dialog element? Why don't we have to wait until the closing promise is resolved? The `_newDialog` name is confusing to me here. Should `_newDialog` be `_topMostDialog`?
> 
> It seems to me like most of the SubDialog.prototype.close code can be removed, since we don't have to worry about re-using anymore, right?

Ideally we should be able to remove the dialog element when we close it, but a lot of tests are closely coupled to rely on the existence of the element.
(Assignee)

Comment 25

2 months ago
mozreview-review-reply
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review132822

> This is a case where the difference between `_newDialog` and `_topDialog` are important but it's not immediately clear to me that this is correct partly due to naming.
> 
> Can you explain this to me if you don't end up cleaning this up based on part 2 suggestions?

I've renamed `_newDialog` to `_preloadDialog`. Hope this would make it more understandable.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 months ago
mozreview-review-reply
Comment on attachment 8852059 [details]
Bug 1340987 - (Part 1) Use arrow functions in gSubDialog.

https://reviewboard.mozilla.org/r/124284/#review129202

> Note that you will need to make the change to in-content-old too. Same for subdialos.js I think though maybe we can unfork that file?

Sorry I don't really understand what you meant by unfork the file. I thought I'd need to manually apply the same changes to the in-content-old copy? or is there a easier way?
(Assignee)

Updated

2 months ago
Attachment #8857070 - Flags: review?(MattN+bmo)
(Assignee)

Updated

2 months ago
Attachment #8857071 - Flags: review?(MattN+bmo)
(Assignee)

Updated

2 months ago
Attachment #8857072 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 months ago
mozreview-review-reply
Comment on attachment 8852059 [details]
Bug 1340987 - (Part 1) Use arrow functions in gSubDialog.

https://reviewboard.mozilla.org/r/124284/#review129202

> Sorry I don't really understand what you meant by unfork the file. I thought I'd need to manually apply the same changes to the in-content-old copy? or is there a easier way?

I've added Part 5 which applies the changes to in-content-old, and unforked subdialogs.js, as well as fixing all the tests that are affected.
Comment hidden (mozreview-request)
(Assignee)

Comment 37

2 months ago
mozreview-review
Comment on attachment 8859641 [details]
Bug 1340987 - (Part 5) Apply changes for subdialogs.js to in-content and fix tests.

https://reviewboard.mozilla.org/r/131662/#review134792

::: commit-message-ec5f7:1
(Diff revision 2)
> +Bug 1340987 - (Part 5) Apply changes for subdialogs.js to in-content-old and fix tests. r=MattN

Rather than unforking subdialogs.js, it's probably better to apply the changes to the `in-content-old/subdialog.js` file.

Since `subdialog.js` references `in-content(-old)/preferences.css` and `in-content(-old)/dialog.css`, unforking it would mean we need to stick to one version of the stylesheets. It would be problematic if the styles related to subdialogs are required to be different for the new and old preferences.
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review135154

Looks good. Only minor comments.

::: commit-message-551f3:1
(Diff revision 4)
> +Bug 1340987 - (Part 2) Make SubDialog stackable. r=MattN

Nit: Maybe add the word "Preference" before SubDialog for clarity

::: browser/components/preferences/in-content/subdialogs.js:10
(Diff revision 4)
>  /* import-globals-from ../../../base/content/utilityOverlay.js */
>  /* import-globals-from preferences.js */
>  
>  "use strict";
>  
> -var gSubDialog = {
> +function SubDialog({template, root, id}) {

I think a JSDoc comment for the constructor would be great or maybe it's because the argument/member names are a bit terse…

::: browser/components/preferences/in-content/subdialogs.js:12
(Diff revision 4)
>  
>  "use strict";
>  
> -var gSubDialog = {
> +function SubDialog({template, root, id}) {
> +  this._template = template;
> +  this._root = root;

I think `parentElement`/`stackParent` or something like that would be a better argument/member name.

::: browser/components/preferences/in-content/subdialogs.js:34
(Diff revision 4)
>      "chrome://browser/skin/preferences/in-content/dialog.css",
>    ],
>    _resizeObserver: null,
> +  _template: null,
> +  _id: null,
> +  _title: null,

Nit: _titleElement

::: browser/components/preferences/in-content/subdialogs.js:38
(Diff revision 4)
> +  _id: null,
> +  _title: null,
> +  _closeButton: null,
> +  _root: null,
>  
>    init() {

May as well make this private (underscore prefixed) since it doesn't make sense to call outside the constructor… I kinda wonder if it would be better inlined in the constructor but I kinda understand the separation.

::: browser/components/preferences/in-content/subdialogs.js:473
(Diff revision 4)
> +   * @type {Array}
> +   */
> +  _dialogs: [],
> +  _dialogStack: null,
> +  _dialogTemplate: null,
> +  _dialogID: 0,

`_nextDialogID`? As-is I could read it as being the ID for this `gSubDialog` instance which doesn't really make sense.

::: browser/components/preferences/in-content/subdialogs.js:533
(Diff revision 4)
> +      // XXX: When a top-most dialog is closed, we reuse the closed dialog and
> +      //      remove the preloadDialog. This is a temporary solution before we
> +      //      rewrite all the test cases.

Please include a bug number for this to be fixed.
Attachment #8857070 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review135162

Looks good. I just need to finish reviewing head.js.

::: browser/components/preferences/in-content/tests/browser_siteData2.js:33
(Diff revision 4)
>    // Test the initial state
>    assertAllSitesListed();
>  
>    // Test the "Cancel" button
>    settingsDialogClosePromise = promiseSettingsDialogClose();
> -  frameDoc = doc.getElementById("dialogFrame").contentDocument;
> +  frameDoc = content.gSubDialog._topDialog._frame.contentDocument;

Instead of `content` can you use `gBrowser.selectedBrowser.contentWindow` (feel free to alias to a `let`) since `content` is a magical alias that may lead people to assume this code is running in a ContentTask or in the frame scope but it's running in the ChromeWindow scope.

::: browser/components/preferences/in-content/tests/head.js:40
(Diff revision 4)
>  function openAndLoadSubDialog(aURL, aFeatures = null, aParams = null, aClosingCallback = null) {
>    let promise = promiseLoadSubDialog(aURL);
> +  executeSoon(() => {
> -  content.gSubDialog.open(aURL, aFeatures, aParams, aClosingCallback);
> +    content.gSubDialog.open(aURL, aFeatures, aParams, aClosingCallback);
> +  });

What's this for? It may deserve a comment but I'm not sure this change is a good idea yet.
Comment on attachment 8859641 [details]
Bug 1340987 - (Part 5) Apply changes for subdialogs.js to in-content and fix tests.

https://reviewboard.mozilla.org/r/131662/#review135166

rs=me assuming this is just unforking stuff. Please reflect any changes made in earlier commits before landing.
Attachment #8859641 - Flags: review?(MattN+bmo) → review+
(Assignee)

Updated

2 months ago
Blocks: 1359023
(Assignee)

Comment 41

2 months ago
mozreview-review-reply
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review135154

Thanks a lot Matt, I've made changes based on your recommendations.

> May as well make this private (underscore prefixed) since it doesn't make sense to call outside the constructor… I kinda wonder if it would be better inlined in the constructor but I kinda understand the separation.

I thought about this but don't really have a good reason for separating the two, so I think I'll just move the init logic to the constructor.
(Assignee)

Comment 42

2 months ago
mozreview-review-reply
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review135162

> Instead of `content` can you use `gBrowser.selectedBrowser.contentWindow` (feel free to alias to a `let`) since `content` is a magical alias that may lead people to assume this code is running in a ContentTask or in the frame scope but it's running in the ChromeWindow scope.

Ok I'll do that.

> What's this for? It may deserve a comment but I'm not sure this change is a good idea yet.

I found that without it, `browser/components/preferences/in-content/tests/browser_connection.js` would crash when calling `content.gSubDialog.open`, but I haven't been able to pin point the cause of it...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8857071 [details]
Bug 1340987 - (Part 3) Fix browser-chrome-mochitest for gSubDialog.

https://reviewboard.mozilla.org/r/128968/#review136648
Attachment #8857071 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review137046

::: browser/components/preferences/in-content/tests/head.js:42
(Diff revision 5)
> +  executeSoon(() => {
> -  content.gSubDialog.open(aURL, aFeatures, aParams, aClosingCallback);
> +    content.gSubDialog.open(aURL, aFeatures, aParams, aClosingCallback);
> +  });

I think we should debug this more. What was the stack of the crash? Did it crash locally or only on Try?
Attachment #8857072 - Flags: review?(MattN+bmo)
(Assignee)

Comment 49

2 months ago
mozreview-review-reply
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review137046

> I think we should debug this more. What was the stack of the crash? Did it crash locally or only on Try?

It crashes both locally and on Try. Here's a try I just did: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb2586fb8d0f4e713cc80216ab246a77e99c403

It happens when `openAndLoadSubDialog` is used, so there are 3 tests that are affected: `browser_connection.js`, `browser_connection_bug388287.js`, and `browser_proxy_backup.js`. It crashes at calling `content.gSubDialog.open`, but the function isn't `undefined`. In the crash log is does say `Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS`. Wonder if you've encountered something similar to this?
(Assignee)

Comment 50

2 months ago
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

Sorry I thought mozreview have reset the review flag but turns out I should do so manually here. Please let me know if you'd like to discuss about this patch on IRC or Vidyo. Thanks Matt!
Attachment #8857072 - Flags: review?(MattN+bmo)
(In reply to Scott Wu [:scottwu] from comment #49)
> Comment on attachment 8857072 [details]
> Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related
> to gSubDialog.
> 
> https://reviewboard.mozilla.org/r/128970/#review137046
> 
> > I think we should debug this more. What was the stack of the crash? Did it crash locally or only on Try?
> 
> It crashes both locally and on Try. Here's a try I just did:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2cb2586fb8d0f4e713cc80216ab246a77e99c403
> 
> It happens when `openAndLoadSubDialog` is used, so there are 3 tests that
> are affected: `browser_connection.js`, `browser_connection_bug388287.js`,
> and `browser_proxy_backup.js`. It crashes at calling
> `content.gSubDialog.open`, but the function isn't `undefined`. In the crash
> log is does say `Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS`.
> Wonder if you've encountered something similar to this?

The useful part is the stack in debug builds which are at: https://treeherder.mozilla.org/logviewer.html#?job_id=94724243&repo=try&lineNumber=4214-4254
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review139498

::: browser/components/preferences/in-content/preferences.xul:200
(Diff revision 5)
> -          <button id="dialogClose"
> +        <button class="dialogClose close-icon"
> -                  class="close-icon"
> -                  aria-label="&preferencesCloseButton.label;"/>
> +                aria-label="&preferencesCloseButton.label;"/>
> -        </caption>
> +      </caption>
> -        <browser id="dialogFrame"
> +      <browser class="dialogFrame"
> -                 name="dialogFrame"
> +               name="dialogFrame"

@name should maybe be deleted since it's never used as-is?

::: browser/components/preferences/in-content/subdialogs.js:27
(Diff revision 5)
> +  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}`);

Can you switch this to use .name instead?
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review137046

> It crashes both locally and on Try. Here's a try I just did: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb2586fb8d0f4e713cc80216ab246a77e99c403
> 
> It happens when `openAndLoadSubDialog` is used, so there are 3 tests that are affected: `browser_connection.js`, `browser_connection_bug388287.js`, and `browser_proxy_backup.js`. It crashes at calling `content.gSubDialog.open`, but the function isn't `undefined`. In the crash log is does say `Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS`. Wonder if you've encountered something similar to this?

The crash report points to this line: https://hg.mozilla.org/mozilla-central/annotate/a374c3546993/dom/base/nsGlobalWindow.cpp#l3993 but that doesn't make much sense to me. Can you attach a native debugger with `./mach run --debug` and set a breakpoint on that line and see what line it actually crashes on. I'm wondering if some inlining, optimization, or preprocessor is changing things a bit.
Comment on attachment 8852059 [details]
Bug 1340987 - (Part 1) Use arrow functions in gSubDialog.

https://reviewboard.mozilla.org/r/124282/#review139502

::: browser/components/preferences/in-content-old/subdialogs.js:29
(Diff revision 8)
> +  parentElement.appendChild(this._overlay);
> +  this._overlay.hidden = false;

Is there a reason for the ordering of these two lines? There's a chance it's related to the crash if an XBL binding is involved.
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review137046

> The crash report points to this line: https://hg.mozilla.org/mozilla-central/annotate/a374c3546993/dom/base/nsGlobalWindow.cpp#l3993 but that doesn't make much sense to me. Can you attach a native debugger with `./mach run --debug` and set a breakpoint on that line and see what line it actually crashes on. I'm wondering if some inlining, optimization, or preprocessor is changing things a bit.

Making a minimal test case by commenting out test and product code until it stops crashing would help narrow down the cause. It's related to openDialog and the arguments passed to it. Maybe an argument changed types and can't be handled anymore?
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review139514

Needinfo me if you need assistance debugging.
Attachment #8857072 - Flags: review?(MattN+bmo)

Comment 57

2 months ago
Created attachment 8865397 [details]
debug log

Here is the stack of crash, still need to find out why. For my brief skim, it seems like we hit a assertion[1] when doing window.openDialog().

[1] http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4048
(In reply to John Dai[:jdai] from comment #57)
> Created attachment 8865397 [details]
> debug log
> 
> Here is the stack of crash, still need to find out why. For my brief skim,
> it seems like we hit a assertion[1] when doing window.openDialog().
> 
> [1]
> http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4048

Can you use a permalink since the line has moved now?
Flags: needinfo?(jdai)

Comment 59

2 months ago
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #58)
> (In reply to John Dai[:jdai] from comment #57)
> > Created attachment 8865397 [details]
> > debug log
> > 
> > Here is the stack of crash, still need to find out why. For my brief skim,
> > it seems like we hit a assertion[1] when doing window.openDialog().
> > 
> > [1]
> > http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4048
> 
> Can you use a permalink since the line has moved now?

Here is the permalink. 
http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/dom/base/nsGlobalWindow.cpp#4046
Flags: needinfo?(jdai)
(Assignee)

Comment 60

a month ago
mozreview-review-reply
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review139498

> @name should maybe be deleted since it's never used as-is?

Yes that could be removed.

> Can you switch this to use .name instead?

`.name` wouldn't work here because it's not shown in the document yet.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 66

a month ago
Since John is caught up in other M3 bugs, and don't have the capacity to thoroughly trace the crash that occur in the tests. He had some ideas but nothing conclusive yet. I'm also solving bugs related to select element. Wonder if we could settle with the workaround for the time being? I've left a comment for future reference: https://reviewboard.mozilla.org/r/128970/diff/5-6/

Some test files have changed quite a bit since the mass replace of `async & await` so I tried rebase it first and pushed to try. The result seems okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38d6ae099200bf2d99ab1b9a24035763ead1d439

I haven't pushed the rebased code because it would mess up the interdiff quite a bit.
Flags: needinfo?(MattN+bmo)
(Assignee)

Updated

25 days ago
Whiteboard: [form autofill:M2] → [form autofill:M2] ETA:612
Created attachment 8874274 [details]
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

Review commit: https://reviewboard.mozilla.org/r/145644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145644/
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review149554

::: browser/components/preferences/in-content/tests/head.js:42
(Diff revision 6)
> +  // 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);
> +  });

See attachment 8874274 [details] for an explanation and potential fix.
Attachment #8857072 - Flags: review?(MattN+bmo)
I'm pretty sure the problem is because the frame/browser isn't ready when we call openDialog, so simply waiting for it to be ready can fix the problem. See comment 67.
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 75

22 days ago
mozreview-review
Comment on attachment 8857070 [details]
Bug 1340987 - (Part 2) Make Preference SubDialog stackable.

https://reviewboard.mozilla.org/r/128966/#review149758

::: browser/components/preferences/in-content-new/subdialogs.js:564
(Diff revision 7)
> +      dialog._overlay.remove();
> +      this._dialogs.splice(this._dialogs.indexOf(dialog), 1);
> +    }
> +
> +    if (this._topDialog) {
> +      fm.moveFocus(this._topDialog._frame.contentWindow, null, fm.MOVEFOCUS_FIRST, fm.FLAG_BYKEY);

Move focus to the top dialog.

::: browser/components/preferences/in-content-new/subdialogs.js:568
(Diff revision 7)
> +    if (this._topDialog) {
> +      fm.moveFocus(this._topDialog._frame.contentWindow, null, fm.MOVEFOCUS_FIRST, fm.FLAG_BYKEY);
> +      this._topDialog._overlay.setAttribute("topmost", true);
> +      this._topDialog._addDialogEventListeners();
> +    } else {
> +      fm.moveFocus(window, null, fm.MOVEFOCUS_ROOT, fm.FLAG_BYKEY);

Move focus to the window.
(Assignee)

Comment 76

22 days ago
Thanks Matt, the solution you suggested works nicely. I've rebased the patches to central, and running try now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda97fc316dd12aa89796aad5a0900d618813772

One thing I added was moving focus when opening and closing dialogs. Without it, keyboard shortcut would not work properly when dialog is closed.
Comment on attachment 8857072 [details]
Bug 1340987 - (Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog.

https://reviewboard.mozilla.org/r/128970/#review149958
Attachment #8857072 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 78

21 days ago
Thanks Matt! Checking it in.
Keywords: checkin-needed

Comment 79

21 days ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e843dab00aa7
(Part 1) Use arrow functions in gSubDialog. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2eca7036eb36
(Part 2) Make Preference SubDialog stackable. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7860182ed49d
(Part 3) Fix browser-chrome-mochitest for gSubDialog. r=MattN
https://hg.mozilla.org/integration/autoland/rev/fb8ec9403cd5
(Part 4) Fix browser-chrome-mochitest for other tests related to gSubDialog. r=MattN
https://hg.mozilla.org/integration/autoland/rev/73f9f9a12e35
(Part 5) Apply changes for subdialogs.js to in-content and fix tests. r=MattN
Keywords: checkin-needed

Comment 80

21 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e843dab00aa7
https://hg.mozilla.org/mozilla-central/rev/2eca7036eb36
https://hg.mozilla.org/mozilla-central/rev/7860182ed49d
https://hg.mozilla.org/mozilla-central/rev/fb8ec9403cd5
https://hg.mozilla.org/mozilla-central/rev/73f9f9a12e35
Status: NEW → RESOLVED
Last Resolved: 21 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.