Closed Bug 1495723 Opened 2 years ago Closed 2 years ago
Firefox Sync not working until manually visiting accounts
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 ?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1fa05a1ba762 Add support for importing extra symbols to `defineLazyGlobalGetters`. r=kmag
I think this is too late for 62, but 63 is merging to release in 3 weeks.
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+
2 years ago
You need to log in before you can comment on or make changes to this bug.