1044700 - Firefox 31's Sync fails to sync addons on Windows

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Martin Pecka]

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 < [e-mail]:283:1 < [e-mail]:866:9 < [e-mail]:742:7 < [e-mail]:99:7 < makeSpinningCallback/[e-mail]:141:32 < [e-mail]:231:5 < [e-mail]:201:5 < [e-mail]:1479:7 < [e-mail]:141:15 < [e-mail]:655:5 < [e-mail]:199:7 < [e-mail]:149:13 < [e-mail]:1254:7 < [e-mail]:141:15 < [e-mail]:96:9 < [e-mail]:1248:1 < sync/<@service.js:1239:7 < [e-mail]:70:9 < [e-mail]:1227:5



Expected results:

My point of view is that there are unescaped backslashes in the path: "D:\Firefox\\Profiles\\fhkjhfsdkjj.peci1\weave".

Comment 1

[Martin Pecka]

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.

Comment 2

[Martin Pecka]

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).

Comment 3

[Martin Pecka]

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.

Comment 4

[Gregory Szorc [:gps]]

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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making `Path.join` and `makeDir` more resilient to empty/non-normalized paths

Comment 7

[Nathan Froyd [:froydnj]]

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.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making `Path.join` and `makeDir` more resilient to empty/non-normalized paths, v2

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.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Note: cannot push to try atm (bug 1040308). Since I'm on PTO, I'm not sure I'll be able to follow up.

Comment 10

[Nathan Froyd [:froydnj]]

(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.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a4462ba437f5
TreeHerder: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4462ba437f5

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Nathan: I believe that `foo = []` is a bit more usual, so I can revert to it if you wish.

Comment 13

[Nathan Froyd [:froydnj]]

If we had `foo = []` before, why don't we keep that.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making `Path.join` and `makeDir` more resilient to empty/non-normalized paths, v3

Applying feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=aaf12ef1eda6

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/abbd3bdde0ac

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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