Firefox Sync not working until manually visiting accounts.firefox.com

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: ragimiri, Assigned: lina)

Tracking

({regression})

62 Branch
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

8 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180920131237

Steps to reproduce:

1. start Firefox

2. in hamburger menu check there is icon and name of Firefox account

3. wait for automatic sync or run it manually


Actual results:

Sync is not successful.

When I click on Firefox account icon and visit accounts.firefox.com I can run sync manually without problem.


Expected results:

Sync should work without manually visiting accounts.firefox.com.
>ERROR	Sync encountered a login error
Component: Untriaged → Sync
Did you reset your master password or did this just start at some point ?
Flags: needinfo?(ragimiri)
Assignee

Comment 3

8 months ago
I think the error is a side effect of switching over to `defineLazyGlobalGetters`.

`Cu.importGlobalProperties(["fetch"])` imports `fetch`, `Headers`, `Request`, and `Response` into the global scope (https://searchfox.org/mozilla-central/rev/6ddb5fb144993fb5de044e2e8d900d7643b98a4d/js/xpconnect/src/Sandbox.cpp#348-350), but the lazy getter only runs when accessing the `fetch` symbol (https://searchfox.org/mozilla-central/rev/6ddb5fb144993fb5de044e2e8d900d7643b98a4d/js/xpconnect/loader/XPCOMUtils.jsm#173-182 )...which means, if we're the first caller to import `fetch`, we'll call `_buildHeaders`, then throw because `importGlobalProperties` hasn't run yet, so `Headers` isn't in scope.
Blocks: 1464548
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ragimiri)
Assignee

Comment 4

8 months ago
Some global imports, like `fetch`, expose multiple symbols. This patch
ensures we can define lazy getters for those symbols, by mapping the
extra symbol name to the main import name.

Comment 5

8 months ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fa05a1ba762
Add support for importing extra symbols to `defineLazyGlobalGetters`. r=kmag
Assignee

Comment 6

8 months ago
I think this is too late for 62, but 63 is merging to release in 3 weeks.
Assignee: nobody → lina

Comment 7

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fa05a1ba762
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee

Comment 8

8 months ago
Comment on attachment 9013780 [details]
Add support for importing extra symbols to `defineLazyGlobalGetters`.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1464548

User impact if declined: Sync will continuously fail if we hit the edge case in comment 3.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is tricky to reproduce manually and in xpcshell, because it requires loading Sync before any other module that imports the `fetch` function. Typically, I think Activity Stream loads first, so most folks won't see this bug...but I'm not sure that order is deterministic.

However, this is a small patch, and I've verified it works locally by removing the `if` check in https://searchfox.org/mozilla-central/rev/6ddb5fb144993fb5de044e2e8d900d7643b98a4d/js/xpconnect/loader/XPCOMUtils.jsm#176,178, then running the Sync tests.

String changes made/needed:
Attachment #9013780 - Flags: approval-mozilla-beta?
Comment on attachment 9013780 [details]
Add support for importing extra symbols to `defineLazyGlobalGetters`.

Minimal patch fixing a 63 Sync regression, uplift approved for 63 beta 12, thanks.
Attachment #9013780 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.