1555198 - The callers of buildDefaultEngineDropDown() do not await its result

Status: RESOLVED WORKSFORME

(Firefox :: Search, defect, P1)

Description

[(no longer active)]

buildDefaultEngineDropDown() is an async function but its callers call it as a normal function so the promise that it returns may get rejected for example because the window object it is running under gets destroyed too quickly.

This can be seen when running browser_cookie_exceptions_addRemove.js locally:

mochitest-browser
~~~~~~~~~~~~~~~~~
Ran 358 checks (357 subtests, 1 tests)
Expected results: 356
Unexpected results: 2
  subtest: 2 (2 fail)

Unexpected Results
------------------
browser/components/preferences/in-content/tests/browser_cookie_exceptions_addRemove.js
  FAIL A promise chain failed to handle a rejection: 2147500037 - stack: buildDefaultEngineDropDown@chrome://browser/content/preferences/in-content/search.js:184:3
async*init@chrome://browser/content/preferences/in-content/search.js:40:10
init@chrome://browser/content/preferences/in-content/preferences.js:62:22
async*initializeCategories@chrome://browser/content/preferences/in-content/findInPage.js:87:26
async*init/<@chrome://browser/content/preferences/in-content/findInPage.js:45:45
requestIdleCallback handler*init@chrome://browser/content/preferences/in-content/findInPage.js:45:14
init_all@chrome://browser/content/preferences/in-content/preferences.js:87:22
EventListener.handleEvent*@chrome://browser/content/preferences/in-content/preferences.js:68:10
Rejection date: Tue May 14 2019 11:45:44 GMT-0400 (Eastern Daylight Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
Stack trace:
resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1140
chrome://mochikit/content/browser-test.js:Tester_execTest:1144
chrome://mochikit/content/browser-test.js:nextTest/<:1005
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
  FAIL A promise chain failed to handle a rejection: 2147500037 - stack: (No stack available.)
Rejection date: Tue May 14 2019 11:45:44 GMT-0400 (Eastern Daylight Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
Stack trace:
resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1140
chrome://mochikit/content/browser-test.js:Tester_execTest:1144
chrome://mochikit/content/browser-test.js:nextTest/<:1005
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

Comment 1

[:Gijs (he/him)]

Mike, this is a regression from bug 1524593, can you or Dale take a look please?

Comment 2

[:Gijs (he/him)]

This is a regression, and it's unclear whether the fact that e.g. init() doesn't wait here could cause real issues - not so much the "oh no something rejected" test stuff (which is easy enough to shut up by adding .catch(() => {}) to things), but because the callsites don't seem to care they're calling an async function ie they don't wait for it to complete.

Still waiting on Mike/Dale to pick this up, as I can't tell from the surrounding code what the right way forward here is. This is ultimately search's code, and a result of the cedar mass landing, so moving over there.

Comment 3

[Dale Harvey (:daleharvey)]

Will take a look at this

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:daleharvey, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Dale Harvey (:daleharvey)]

Ehsan are you still able to get the uncaught exception when running these tests locally? what platform / type of build are you doing? havent been able to get this error to throw for me on windows / osx, artifact / full builds etc

There isnt any problem with the callers not waiting on the menulist being rendered, most of the calls are from async observers anyway, init() shows it but doesnt depend on it being shown later on, I think we are ok with just making sure this tears down without any errors

I dont want to add .catch() to every call though, seems a bit excessive and likely to hide other real exceptions that we want to see and fix so looking for a more direct fix, I dont the full details of how window destruction happens, if I add await sleep(1000); in that function then close the window while its waiting I dont get any error so there must be some time between the window object being destroyed and the script being killed, guessing an if (window) @ https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/search.js#180 would be enough, but not certain and would like to verify, either by finding a failing test or by soneone who is more confident about what the error is

Comment 6

[(no longer active)]

It happened on Linux (IIRC optimized) while running browser_cookie_exceptions_addRemove.js locally around the time I filed it.

Comment 7

[Dale Harvey (:daleharvey)]

So spent a bunch of time trying to reproduce this and couldnt, nobody is seeing it right now and havent seen any notifications of failures on try

There isnt any problem with the callers not awaiting, the drop down is just populated async, be good to ensure no errors get through but I cant see that any are at the moment, at the point window doesnt exist not sure the script should still be executing so possibly something changed around there and fixed this, can look into it again if someone sees it