Closed Opened 8 years ago Closed 8 years ago

# Firefox 31's Sync fails to sync addons on Windows

x86_64
Windows 7
Not set
normal

RESOLVED FIXED
mozilla34

## Attachments

### (1 file, 2 obsolete files)

 5.81 KB, patch Yoric : review+ Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Since the update to 31, FF has started to display the message that sync has failed due to unknown error. In the sync log I see this:

1406502918706	Sync.Status	DEBUG	Status for engine addons: error.engine.reason.unknown_fail
1406502918706	Sync.Status	DEBUG	Status.service: success.status_ok => error.sync.failed_partial
1406502918706	Sync.ErrorHandler	DEBUG	addons failed: Error: Error: Incorrect use of option |from|: D:\Firefox\\Profiles\\fhkjhfsdkjj.peci1\weave is not a descendant of D:\\Firefox\\Profiles\\fhkjhfsdkjj.peci1 (resource://gre/modules/osfile/osfile_shared_front.jsm:556) JS Stack trace: post/<@osfile_async_front.jsm:461:11 < TaskImpl_run@Task.jsm:283:1 < Handler.prototype.process@Promise-backend.js:866:9 < this.PromiseWalker.walkerLoop@Promise-backend.js:742:7 < waitForSyncCallback@async.js:99:7 < makeSpinningCallback/callback.wait@async.js:141:32 < _refreshReconcilerState@addons.js:231:5 < _syncStartup@addons.js:201:5 < SyncEngine.prototype._sync@engines.js:1479:7 < WrappedNotify@util.js:141:15 < Engine.prototype.sync@engines.js:655:5 < _syncEngine@enginesync.js:199:7 < sync@enginesync.js:149:13 < onNotify@service.js:1254:7 < WrappedNotify@util.js:141:15 < WrappedLock@util.js:96:9 < _lockedSync@service.js:1248:1 < sync/<@service.js:1239:7 < WrappedCatch@util.js:70:9 < sync@service.js:1227:5

Expected results:

My point of view is that there are unescaped backslashes in the path: "D:\Firefox\\Profiles\\fhkjhfsdkjj.peci1\weave".
Component: Untriaged → Firefox Sync: Backend
Product: Firefox → Mozilla Services
Version: 31 Branch → unspecified
Flags: needinfo?(gps)
My second notebook with the same system configuration (Win 7, FF profile saved on non-system parition D: ) doesn't suffer from this problem. On the other hand, I don't know of any event that could have damaged data on the first notebook's drive, nor do I have suspicion for a virus infection.
I've tried to reinstall FF on the computer with problems and it did not help. (It was not a full cleanup and fresh install, cause I don't want to lose my settings).
Ok, so a correction to what I've posted previously. The second computer (which I reported not to suffer from this bug) now shows the same buggy behavior I originally described.
This is a regression from bug 988301, which introduced OS.File.makeDir() into Sync code.

This is arguably a bug in OS.File itself. The "from" parameter and the error getting thrown were introduced in bug 934283.

Yoric: This feels like your bug to triage.
Depends on: 988301, 934283
Flags: needinfo?(gps) → needinfo?(dteller)
Keywords: regression
Indeed, this D:\Firefox\\Profiles\\fhkjhfsdkjj.peci1\weave with varying number of backslashes looks pretty weird. With these backslashes, it is indeed not a descendant of D:\\Firefox\\Profiles\\fhkjhfsdkjj.peci1. I suspect that some code is not using OS.Path.

After a quick look through DXR, I assume that the caller is http://dxr.mozilla.org/mozilla-central/source/services/sync/modules/util.js#363-364 . The from option is apparently correct, but the path arg is apparently somehow wrong. I haven't managed to reproduce the issue just yet, though.
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
Assignee: nobody → dteller
Attachment #8468479 - Flags: review?(nfroyd)
Flags: needinfo?(dteller)
Comment on attachment 8468479 [details] [diff] [review]
Making Path.join and makeDir more resilient to empty/non-normalized paths

Review of attachment 8468479 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/ospath_win.jsm
@@ +135,5 @@
>   *  var path = OS.Path.join(tmpDir, "foo", "bar");
>   *
>   * Under Windows, this will return "\$TMP\foo\bar".
> + *
> + * Empth components are ignored, i.e. OS.Path.join("foo", "", "bar) is the

"Empty"

@@ +156,5 @@
>        let component = trimBackslashes(subpath.slice(drive.length));
>        if (component) {
>          paths = [component];
>        } else {
> +        paths.length = 0;

Do you think this is more idiomatic?  I would tend to prefer the [] version, but am willing to defer to people who write more JavaScript than I do.
Attachment #8468479 - Flags: review?(nfroyd) → review+
Sorry, missing qref.

To answer your question, the difference between foo = [] and foo.length = 0 is that the first one allocates a new array, while the second one empties an existing array. Not really the same operation.
Attachment #8468479 - Attachment is obsolete: true
Attachment #8468731 - Flags: review?(nfroyd)
Note: cannot push to try atm (bug 1040308). Since I'm on PTO, I'm not sure I'll be able to follow up.
(In reply to David Rajchenbach Teller [:Yoric] (away until August 20th - use "needinfo") from comment #8)
> To answer your question, the difference between foo = [] and foo.length =
> 0 is that the first one allocates a new array, while the second one empties
> an existing array. Not really the same operation.

Well, yes, but in both cases, you wind up with an array that is empty.  So the question is what is the idiomatic way of achieving that?  The second one may be somewhat faster, but I think the first one is clearer in communicating what you actually want to do.
Attachment #8468731 - Flags: review?(nfroyd) → review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=a4462ba437f5
TreeHerder: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4462ba437f5
Nathan: I believe that foo = [] is a bit more usual, so I can revert to it if you wish.
Flags: needinfo?(nfroyd)
If we had foo = [] before, why don't we keep that.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nfroyd)
Applying feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=aaf12ef1eda6
Attachment #8468731 - Attachment is obsolete: true
Attachment #8475932 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/abbd3bdde0ac
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/abbd3bdde0ac
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla34
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.