1413989 - setTimeout is undefined in MigrationUtils.getRowsFromDBWithoutLocks

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P1)

Description

[Alex Thayer [:alexical] (she/her)]

This breaks our retry logic in Chrome migrations.

Comment 1

[Matthew N. [:MattN]]

The setTimout was originally added in bug 1285041 in ChromeProfileMigrator.js and was moved to MigrationUtils.jsm in bug 1305770.

Doug, I see you're assigned, are you working on this?

You should just be able to import Timer.jsm. Testing it will be more difficult as you'll need to force an exception. See the test_Chrome* files in https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/unit

Comment 2

[:Gijs (he/him)]

Ugh, I am dumb sometimes. How isn't this failing eslint? :-(

Comment 3

[:Gijs (he/him)]

I also wonder if this is the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=1332225#c34 .

Comment 4

[Mark Banner (:standard8)]

(In reply to :Gijs (slow, PTO recovery mode) from comment #2)
> Ugh, I am dumb sometimes. How isn't this failing eslint? :-(

That's bug 1369722. With the ESLint 4 upgrade next week, it'll be possible that we can do it in a sane way now.

Comment 5

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs (slow, PTO recovery mode) from comment #3)
> I also wonder if this is the cause of
> https://bugzilla.mozilla.org/show_bug.cgi?id=1332225#c34 .

I couldn't find a way for this to result in the migrator hanging. Forcing an error in order to hit this block just resulted in nothing being imported.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #1)
> The setTimout was originally added in bug 1285041 in
> ChromeProfileMigrator.js and was moved to MigrationUtils.jsm in bug 1305770.
> 
> Doug, I see you're assigned, are you working on this?
> 
> You should just be able to import Timer.jsm. Testing it will be more
> difficult as you'll need to force an exception. See the test_Chrome* files
> in
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> tests/unit

Woops. I filed this while in the middle looking into the performance of migrations and promptly forgot I had created it and also assigned myself. But yes, I intended to work on it and I'll get a patch up shortly.

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1413989 - include Timer.jsm in MigrationUtils.jsm

setTimeout was previously undefined. Created a test specifically
for MigrationUtils, since trying to test this through an actual
Chrome migration path seemed trickier and more fragile.

Review commit: https://reviewboard.mozilla.org/r/197384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197384/

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8926146 [details]
Bug 1413989 - include Timer.jsm in MigrationUtils.jsm

https://reviewboard.mozilla.org/r/197384/#review202640

Comment 8

[Matthew N. [:MattN]]

Thanks

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d8c4617ad28
include Timer.jsm in MigrationUtils.jsm r=MattN

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c8659b2921a
Backed out 1 changesets for ESLint failure "browser/components/migration/tests/unit/test_MigrationUtils_timedRetry.js" r=backout on a CLOSED TREE

Comment 11

[:Gijs (he/him)]

(In reply to Pulsebot from comment #10)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/0c8659b2921a
> Backed out 1 changesets for ESLint failure
> "browser/components/migration/tests/unit/test_MigrationUtils_timedRetry.js"
> r=backout on a CLOSED TREE

Welp, that will teach me to be pushing other people's patches - sorry!

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 57 but we could still take this patch for 58.

Comment 13

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs (slow, PTO recovery mode) from comment #11)
> Welp, that will teach me to be pushing other people's patches - sorry!

Eh, my bad for not habitually linting it before pushing to review.

Comment 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8926146 [details]
Bug 1413989 - include Timer.jsm in MigrationUtils.jsm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197384/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0280b48ef58
include Timer.jsm in MigrationUtils.jsm r=MattN

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d0280b48ef58