setTimeout is undefined in MigrationUtils.getRowsFromDBWithoutLocks

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dthayer, Assigned: dthayer)

Tracking

({regression})

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

()

Attachments

(1 attachment)

This breaks our retry logic in Chrome migrations.
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
Blocks: 1285041, 1305770
Flags: needinfo?(dothayer)
Keywords: regression
Priority: -- → P1
Ugh, I am dumb sometimes. How isn't this failing eslint? :-(
I also wonder if this is the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=1332225#c34 .
(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.
(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.
Flags: needinfo?(dothayer)
Comment on attachment 8926146 [details]
Bug 1413989 - include Timer.jsm in MigrationUtils.jsm

https://reviewboard.mozilla.org/r/197384/#review202640
Attachment #8926146 - Flags: review?(MattN+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d8c4617ad28
include Timer.jsm in MigrationUtils.jsm r=MattN
Backout by nbeleuzu@mozilla.com:
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
(In reply to Pulsebot from comment #10)
> Backout by nbeleuzu@mozilla.com:
> 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!
Too late for 57 but we could still take this patch for 58.
(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.
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0280b48ef58
include Timer.jsm in MigrationUtils.jsm r=MattN
https://hg.mozilla.org/mozilla-central/rev/d0280b48ef58
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.