1554038 - Quantumbar intermittently stops working (TypeError: result is undefined UrlbarInput.jsm:437:9)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[bgstandaert]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Firefox/68.0

Actual results:

Since upgrading to v68 beta with Quantumbar enabled, the address bar will intermittently stop working (maybe 1 out of 20 times?). What happens is as follows:

1. I'll type something in the address bar, and press tab to move down to a result.
2. Pressing enter will do nothing - the result will still be highlighted and will not load.
3. Every time I press enter, the following error will appear in the browser console:
   TypeError: result is undefined UrlbarInput.jsm:437:9
4. If I close the address bar and re-open it, it will start working again.

I can't make this happen reliably, and there doesn't seem to be anything else that's obviously different between when it works and when it doesn't; if there's something you want me to test to narrow this down further, let me know and I'd be happy to try it.

Comment 1

[Marco Bonardo [:mak]]

Do you have any userChrome.css modifications active?

Comment 2

[bgstandaert]

I do, although I wouldn't expect any of them to affect the address bar. Here's the contents of my userChrome file:

#nav-bar {
    box-shadow: 0 -1px 1px 0 rgba(0,0,0,0.2) !important;
}

#back-button {
    padding-top: 2px !important;
    padding-bottom: 2px !important;
}

#back-button > .toolbarbutton-icon {
    border: none !important;
    background: none !important;
}

#urlbar:not([focused="true"]) {
    border-color: rgba(0,0,0,0.15) !important;;
}

#TabsTitlebar {
    -moz-appearance: none !important;
}

Comment 3

[Drew Willcoxon :adw]

index must be this._queryContext.results.length because otherwise getResult would throw: https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/browser/components/urlbar/UrlbarView.jsm#123

Which is a separate problem: that index check should be checking >= length, not > length...

(I also checked whether we delete any items from the results array, which would be strange, and we don't.)

So the question is, why is resultIndex in pickResult too large?

Comment 4

[Drew Willcoxon :adw]

Or index in getResult might not be an integer, like undefined, but I don't see how that's possible. And there would be an error like 'ReferenceError: reference to undefined property "undefined"' if that were the case.

Comment 5

[Drew Willcoxon :adw]

I can reproduce it: Type something in the urlbar and wait for the query to stop (i.e., for all results appear). Then type another character and very quickly hit tab. You need to change the selection before the results are updated to reflect the new query. The newly selected result needs to be from the previous query. (Seems easiest to do by going from a query that returns history results to one that returns search suggestions.) Then hit enter.

It looks like we don't handle this case at all, where a stale result from the previous query is picked?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #5)

It looks like we don't handle this case at all, where a stale result from the previous query is picked?

We need to pass around UrlbarResults instead of indices to address this. I'm going to post a patch to fix this specific bug, then probably file a followup to further cleanup (potentially getting rid of the resultIndex attribute entirely).

Comment 7

[Dão Gottwald [:dao]]

Attachment: Bug 1554038 - Make UrlbarInput::pickResult and UrlbarController::recordSelectedResult take a UrlbarResult instead of an index and remove UrlbarView::getResult.

Comment 8

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f49b045c24588beeef01008dae0fc66451fd4f5

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e901c73992d
Make UrlbarInput::pickResult and UrlbarController::recordSelectedResult take a UrlbarResult instead of an index and remove UrlbarView::getResult. r=adw

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7e901c73992d

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 9067790 [details]
Bug 1554038 - Make UrlbarInput::pickResult and UrlbarController::recordSelectedResult take a UrlbarResult instead of an index and remove UrlbarView::getResult.

Beta/Release Uplift Approval Request

• User impact if declined: see comment 0
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see comment 5
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): pretty straightforward refactoring
• String changes made/needed:

Comment 12

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #6)

(In reply to Drew Willcoxon :adw from comment #5)

It looks like we don't handle this case at all, where a stale result from the previous query is picked?

We need to pass around UrlbarResults instead of indices to address this. I'm going to post a patch to fix this specific bug, then probably file a followup to further cleanup (potentially getting rid of the resultIndex attribute entirely).

For those following along, I ended up removing resultIndex here.

Comment 13

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello,
Reproduced the issue using steps from comment 5 with Firefox 69.0a1 (20190524214959) on Windows 10 x64, macOS 10.14 and Ubuntu 18.04.
The issue is verified using Firefox 69.0a1 (20190531035909) on Windows 10x64, Ubuntu 18.04 and macOS 10.14.

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9067790 [details]
Bug 1554038 - Make UrlbarInput::pickResult and UrlbarController::recordSelectedResult take a UrlbarResult instead of an index and remove UrlbarView::getResult.

quantumbar fix for 68.0b7

Comment 15

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8536ebb1c16f

Comment 16

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello,
The issue is verified using Firefox 68.0b7 (20190603181408) on Windows 10x64, macOS 10.14, Ubuntu 18.04.