Closed
Bug 1259303
Opened 8 years ago
Closed 8 years ago
Intermittent browser_context_menu.js | application crashed [@ nsAutoCompleteController::OnSearchResult] after Assertion failure: mSearchesOngoing > 0 && mSearches.Contains(aSearch)
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: philor, Assigned: mrbkap)
Details
(Keywords: assertion, intermittent-failure)
Attachments
(1 file, 3 obsolete files)
5.16 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 3•8 years ago
|
||
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
Attachment #8737938 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mrbkap
Comment 4•8 years ago
|
||
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•8 years ago
|
||
Blake, do you know why this recently started to happen?
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 6•8 years ago
|
||
This has been happening since the test was converted to be a browser-chrome test.
Flags: needinfo?(mrbkap)
Comment 7•8 years ago
|
||
I believe this test was always a browser-chrome test though
Assignee | ||
Comment 8•8 years ago
|
||
Ah, no idea why it would have changed, then.
Comment 9•8 years ago
|
||
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
Attachment #8737938 -
Flags: review?(MattN+bmo) → review?(mak77)
Assignee | ||
Comment 10•8 years ago
|
||
There isn't a result to handle here. I renamed the variable. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae46fe6a1a62
Attachment #8739607 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8737938 -
Attachment is obsolete: true
Attachment #8737938 -
Flags: review?(mak77)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 12•8 years ago
|
||
Sorry, I forgot to fold my changes into that last patch.
Attachment #8740215 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8739607 -
Attachment is obsolete: true
Attachment #8739607 -
Flags: review?(MattN+bmo)
Comment 13•8 years ago
|
||
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
Attachment #8740215 -
Flags: review?(MattN+bmo) → feedback+
Assignee | ||
Comment 14•8 years ago
|
||
I have no idea what happened with my patch management for this bug. That patch is definitely not the patch I had tested locally...
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8741998 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8740215 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8741998 -
Flags: review?(MattN+bmo) → review+
Comment hidden (Intermittent Failures Robot) |
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53252c783a43
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•