Closed Bug 1406585 Opened 7 years ago Closed 6 years ago

autocomplete widget remains open if window closed underneath it

Categories

(Toolkit :: Form Manager, defect, P2)

Product:
Toolkit
Component:
Form Manager
Version:
49 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- fix-optional
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mixedpuppy, Assigned: selee)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.
59 bytes, text/x-review-board-request
MattN
: review+
Details
FormAutofill Popup stays in the crashed page
151.76 KB, image/png
Details 
In looking at bug 1406229, I noticed that if the STR is done in a tab that the tab would close but the autocomplete list popup would remain.  My fix for 1406229 causes the same behavior in the browserAction panel.  

I chased this issue down to a lack of messageManager at the time that browser-content.js sends the FormAutoComplete:ClosePopup message.

Basically this will cause the problem:

  input.addEventListener('keydown', e => {
    if (e.key === "Enter") { window.close(); }
  });

Placing the window.close in a timeout allows the popup to close properly.

I'll use the simple fix for the crash in bug 1406229 since we should be able to uplift it to 57.
Component: Autocomplete → Form Manager
Priority: -- → P2
uoops, didn't mean to set a priority, this still needs triage.
Priority: P2 → --
I remember this getting filed by QA but I can't find the bug to dupe it to…
status-firefox57: --- → fix-optional
Whiteboard: [DUPEME]
Blocks: 1407329
Also of note.  Once in this state, the dropdown no longer works in subsequent use.  So it's pretty important to get this triaged and addressed.
I asked the Autofill team to look into this. I agree it's quite bad but since it's not new marking as P2.
Priority: -- → P2
Assignee: nobody → selee
Status: NEW → ASSIGNED
I can reproduce the issue of "Quick Accept-Language Switcher" mentioned in bug 1406229 comment 0.

For the broken autocomplete feature mentioned in comment 3, this is caused by the undefined `messageManager` in [1]. Checking `browser && browser.messageManager` (included in the patch) can fix the broken autocomplete issue but the popup still shows there, so I am trying to find a way to close the popup.

BTW, I found the autocomplete popup will stay on the crash page (attachment 8927890 [details]) when a content page is showing the autocomplete popup. Applying the following command in console can reproduce this:
$ sleep 5; kill -9 <ContentPID>

The patch attachment 8927879 [details] provides a solution to close the popup when "oop-frameloader-crashed" happens.

[1] https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/toolkit/components/satchel/AutoCompletePopup.jsm#316
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

Hi Matt, Macro,

The patch fixes the broken autocomplete feature in the original STR and fixes the "oop-frameloader-crashed" issue as well.
Could you help to take a look this patch?
BTW, I am not sure how to close the popup in the enter keydown case.
May I have your suggestion?
Thank you.
Attachment #8927879 - Flags: feedback?(mak77)
Attachment #8927879 - Flags: feedback?(MattN+bmo)
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review204258

The "message-manager-disconnect" message may be useful for your tab closing issue… not sure

::: browser/base/content/browser.js:1429
(Diff revision 2)
>  
>      this._handleURIToLoad();
>  
>      Services.obs.addObserver(gIdentityHandler, "perm-changed");
>      Services.obs.addObserver(gRemoteControl, "remote-active");
> +    Services.obs.addObserver(gOOPFrameloaderCrashed, "oop-frameloader-crashed");

I think this should move to https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/components/satchel/AutoCompletePopup.jsm#100 and the observer should be part of AutocompletePopup too
Attachment #8927879 - Flags: feedback?(MattN+bmo)
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review204258

> I think this should move to https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/components/satchel/AutoCompletePopup.jsm#100 and the observer should be part of AutocompletePopup too

The observer related codes are moved to AutoCompletePopup.jsm in the latest patch.
"message-manager-disconnect" did sent when the tab closed or crashed, so I use it to replace "oop-frameloader-crashed". Thanks for the suggestion.
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review205270

::: toolkit/components/satchel/AutoCompletePopup.jsm:119
(Diff revision 5)
> +      case "message-manager-disconnect": {
> +        // `setTimeout` is to wait for the initialization of AutoCompletePopup
> +        // when the crash of a content page or Message Manager disconnection
> +        // happens.
> +        setTimeout(() => {

Could you explain a bit more why the setTimeout is necessary? I'm confused about what specifically you mean by "wait for the initialization of AutoCompletePopup" since this observer is inside of AutoCompletePopup and the observer could only be registered after AutoCompletePopup.init

::: toolkit/components/satchel/AutoCompletePopup.jsm:335
(Diff revision 5)
>        null;
> -    if (browser) {
> +    if (browser && browser.messageManager) {
>        browser.messageManager.sendAsyncMessage(msgName, data);

Is this still necessary with the observer? Is it just to avoid a console warning or does it break things? If so, is it necessary only for the crash case or also for the tab close case? 

Sorry, I don't fully understand the flow.

I'm worried that we're going to be hiding an exceptional case and thinking we should at least warn if this is truly exceptional.
Attachment #8927879 - Flags: review?(MattN+bmo)
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review207192

::: toolkit/components/satchel/AutoCompletePopup.jsm:119
(Diff revision 5)
> +      case "message-manager-disconnect": {
> +        // `setTimeout` is to wait for the initialization of AutoCompletePopup
> +        // when the crash of a content page or Message Manager disconnection
> +        // happens.
> +        setTimeout(() => {

Ooh, I should mention more about the details.
There are two ways to reproduce the popup showing still issue:
1. STR mentioned in bug 1406229 comment0:
The popup can be closed by `this.openedPopup.closePopup();` without any error, and `setTimeout` is not needed at all. BTW, `browser.messageManager` is still necessary in `sendMessageToBrowser`.

2. STR in comment8 (see attachment 8927890 [details]):
`setTimeout` here is for the crash page case exactly. 
There is an error without `setTimeout`:
NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]
This error happens in the following code path in `popupBox.hidePopup(cancel);`[1]. I guess it is related to [2] but I have not traced the following code yet.
Sorry for the confusion. My original guessing was about the initialization of content page instead of AutoCompletePopup. However, `setTimeout` solution is not a confident solution. I will figure out a better solution to solve the case 2. e.g., waiting for an exact event.

[1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/toolkit/content/widgets/popup.xml#149
[2] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/layout/xul/nsXULPopupManager.cpp#991

::: toolkit/components/satchel/AutoCompletePopup.jsm:335
(Diff revision 5)
>        null;
> -    if (browser) {
> +    if (browser && browser.messageManager) {
>        browser.messageManager.sendAsyncMessage(msgName, data);

Without checking messageManager, it shows the error `TypeError: browser.messageManager is undefined`. Once the error happened, all autocomplete popups won't work anymore.

`this.sendMessageToBrowser("FormAutoComplete:Focus");`[1] will try to send this message during closing the popup: `AutoCompletePopup.requestFocus();`[2].

For this case, `"message-manager-disconnect"` happens first, then it tries to send "FormAutoComplete:Focus" message. I think it's reasonable to check `browser.messageManager`. I am not sure if we want to check if the message is sent after exact `"message-manager-disconnect"`.

[1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/toolkit/components/satchel/AutoCompletePopup.jsm#328
[2] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/toolkit/components/satchel/AutoCompletePopup.jsm#75
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review208736

Please add a test for this if it's not too hard and file a follow-up for STR 2.

Thanks!

::: toolkit/components/satchel/AutoCompletePopup.jsm:327
(Diff revision 7)
>        null;
> -    if (browser) {
> +    if (browser && browser.messageManager) {

I think we should clean this up and also include an error in the console when the messageManager isn't available:

```js
if (!browser) {
  return;
}

if (browser.messageManager) {
  browser.messageManager.sendAsyncMessage(msgName, data);
} else {
  Cu.reportError(`AutoCompletePopup: No messageManager for message "${msgName}"`);
}
```
Attachment #8927879 - Flags: review?(MattN+bmo) → review+
Blocks: 1424931
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review208736

Thanks for the review! bug 1424931 is filed for STR 2. I am not sure if there is a trivial solution to reproduce this in browser-chrome test. Probably I can close the tab with popup showing.

> I think we should clean this up and also include an error in the console when the messageManager isn't available:
> 
> ```js
> if (!browser) {
>   return;
> }
> 
> if (browser.messageManager) {
>   browser.messageManager.sendAsyncMessage(msgName, data);
> } else {
>   Cu.reportError(`AutoCompletePopup: No messageManager for message "${msgName}"`);
> }
> ```

Fixed in the latest patch.
The patch includes a new test to verify the popup closed after its window/tab closed as well.

Let's wait for the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f34366bbb2a29c2712e30f502762a8b2748da64f
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review213680

::: toolkit/components/satchel/test/browser/browser_close_tab.js:39
(Diff revision 9)
> +
> +    let listener;
> +    let errorLogPromise = new Promise(resolve => {
> +      listener = ({message}) => {
> +        const ERROR_MSG = "AutoCompletePopup: No messageManager for message \"FormAutoComplete:PopupClosed\"";
> +        if(message.includes(ERROR_MSG)) {

There are some eslint issues on Try btw. e.g. space after `if`
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review213684

::: toolkit/components/satchel/test/browser/browser_close_tab.js:26
(Diff revision 9)
> +      input.focus();
> +    });
> +
> +    // show popup
> +    await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);

These two calls will race I think since focus is async but I could be wrong. Please make sure you run the test verification job on try: https://developer.mozilla.org/en-US/docs/Test_Verification
Comment on attachment 8927879 [details]
Bug 1406585 - Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens.

https://reviewboard.mozilla.org/r/199150/#review213700

::: toolkit/components/satchel/test/browser/browser_close_tab.js:26
(Diff revision 9)
> +      input.focus();
> +    });
> +
> +    // show popup
> +    await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);

Here is the verification result for browser_close_tab.js:

./mach mochitest toolkit/components/satchel/test/browser/browser_close_tab.js --verify
...
:::
::: Test verification summary for:
:::
::: toolkit/components/satchel/test/browser/browser_close_tab.js
:::
::: 1. Run each test 10 times in one browser. : Pass
::: 2. Run each test 5 times in a new browser each time. : Pass
::: 3. Run each test 10 times in one browser, in chaos mode. : Pass
::: 4. Run each test 5 times in a new browser each time, in chaos mode. : Pass
:::
::: Test verification PASSED
:::

The result is PASS. BTW, the test is copied from browser_popup_mouseover.js. The try result looks good for the autocomplete part.

::: toolkit/components/satchel/test/browser/browser_close_tab.js:39
(Diff revision 9)
> +
> +    let listener;
> +    let errorLogPromise = new Promise(resolve => {
> +      listener = ({message}) => {
> +        const ERROR_MSG = "AutoCompletePopup: No messageManager for message \"FormAutoComplete:PopupClosed\"";
> +        if(message.includes(ERROR_MSG)) {

Thanks. the patch is updated.
Ooh, I am kind of misunderstanding Matt's comment. Here is another try link for TW:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d03ca3039c3de987dffb98757f493e9fb3cd1c7d
Hey MattN, the TV result looks good. I think it's ready to land it. Thank you.
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2dcdbd994f1
Close the autocomplete popup when the crash of a content page or Message Manager disconnection happens. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c2dcdbd994f1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Too late for Beta58. Mark 58 won't fix.
status-firefox58: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: