1259303 - Intermittent browser_context_menu.js | application crashed [@ nsAutoCompleteController::OnSearchResult] after Assertion failure: mSearchesOngoing > 0 && mSearches.Contains(aSearch)

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Phil Ringnalda (:philor)]

https://treeherder.mozilla.org/logviewer.html#?job_id=24402625&repo=mozilla-inbound

Comment 1

[Intermittent Failures Robot]

9 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 4
* try: 3
* mozilla-central: 1
* fx-team: 1

Platform breakdown:
* linux64: 5
* linux32: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1259303&startday=2016-03-21&endday=2016-03-27&tree=all

Comment 2

[Intermittent Failures Robot]

10 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 6
* try: 3
* mozilla-central: 1

Platform breakdown:
* linux32: 7
* linux64: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1259303&startday=2016-03-28&endday=2016-04-03&tree=all

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed patch

This patch applies on top of the patch in bug 1258921. I have a try push with this patch up at [1]. We basically ignore the StopSearch call from nsAutoCompleteController for login manager async searches. This patch sets some state in nsLoginManager in order to notice that we've been canceled.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=96346c42d3bc

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8737938 [details] [diff] [review]
Proposed patch

Review of attachment 8737938 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +87,5 @@
>      this._prefBranch = Services.prefs.getBranch("signon.");
>      this._prefBranch.addObserver("rememberSignons", this._observer, false);
>  
>      this._remember = this._prefBranch.getBoolPref("rememberSignons");
> +    this._lookupPromise = null;

This variable name isn't very descriptive for nsLoginManager which isn't solely about autocomplete

Comment 5

[Matthew N. [:MattN]]

Blake, do you know why this recently started to happen?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

This has been happening since the test was converted to be a browser-chrome test.

Comment 7

[Matthew N. [:MattN]]

I believe this test was always a browser-chrome test though

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Ah, no idea why it would have changed, then.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8737938 [details] [diff] [review]
Proposed patch

Review of attachment 8737938 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +468,5 @@
> +    this._lookupPromise.then(({ logins, messageManager }) => {
> +                               // If the search was canceled before we got our
> +                               // results, don't bother reporting them.
> +                               if (this._lookupPromise !== lookupPromise) {
> +                                 return;

Marco, is there a better way to detect a stopped search? e.g. one of the status codes on autocomplete results? None of them mention "stop" but maybe something like RESULT_FAILURE is used?

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +774,5 @@
>    if (mLastFormAutoComplete) {
>      mLastFormAutoComplete->StopAutoCompleteSearch();
>      mLastFormAutoComplete = nullptr;
> +  } else {
> +    mLoginManager->StopSearch();

Are we guaranteed that mLoginManager isn't null here? I don't see how

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.1

There isn't a result to handle here. I renamed the variable.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae46fe6a1a62

Comment 11

[Intermittent Failures Robot]

7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 4
* fx-team: 2
* mozilla-central: 1

Platform breakdown:
* linux32: 6
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1259303&startday=2016-04-04&endday=2016-04-10&tree=all

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.1

Sorry, I forgot to fold my changes into that last patch.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8740215 [details] [diff] [review]
Patch v1.1

Review of attachment 8740215 [details] [diff] [review]:
-----------------------------------------------------------------

f+ since I would like the answer about nsFormFillController.cpp

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +461,5 @@
>        previousResult = null;
>      }
>  
>      let rect = BrowserUtils.getElementBoundingScreenRect(aElement);
> +    let autoCompleteLookupPromise = this._autoCompletelookupPromise =

Please be consistent with the casing. You should capitalize the "L" in "Lookup" for the member variable too.

@@ +468,5 @@
> +    autoCompleteLookupPromise.then(({ logins, messageManager }) => {
> +                               // If the search was canceled before we got our
> +                               // results, don't bother reporting them.
> +                               if (this._lookupPromise !== lookupPromise) {
> +                                 return;

I think you forgot to rename these 2 variables to include the autocomplete prefix.

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +774,5 @@
>    if (mLastFormAutoComplete) {
>      mLastFormAutoComplete->StopAutoCompleteSearch();
>      mLastFormAutoComplete = nullptr;
> +  } else {
> +    mLoginManager->StopSearch();

What's your reply to:

(Quoting Matthew N. [:MattN] from comment #9)
> ::: toolkit/components/satchel/nsFormFillController.cpp
> @@ +774,5 @@
> >    if (mLastFormAutoComplete) {
> >      mLastFormAutoComplete->StopAutoCompleteSearch();
> >      mLastFormAutoComplete = nullptr;
> > +  } else {
> > +    mLoginManager->StopSearch();
> 
> Are we guaranteed that mLoginManager isn't null here? I don't see how

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I have no idea what happened with my patch management for this bug. That patch is definitely not the patch I had tested locally...

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.2

Comment 16

[Intermittent Failures Robot]

16 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 10
* try: 4
* fx-team: 2

Platform breakdown:
* linux32: 12
* linux64: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1259303&startday=2016-04-11&endday=2016-04-17&tree=all

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53252c783a43

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53252c783a43