641617 - Bookmarks: Ignore records with query URIs that refer to folders by ID

Status: RESOLVED INVALID

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

See Bug 641074.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Part 1. v1

This patch implements logic to ignore incoming records that refer to Places folder IDs (but aren't tag queries).

There is a tiny piece of unnecessary work if the current proposed plan -- to in-place alter IDs into GUIDs -- does not take place (e.g., for backwards-compatibility reasons), but we'll cross that bridge when mak's built it :)

This could probably do with some belt-and-braces (CrossWeave?) higher-level tests. Right now it's only tested at the "should we apply this?" level, rather than with actual downloaded records.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 2. v1

This extends BookmarksTracker._ignore to examine bookmark URIs, skipping those which contain non-GUID folders. The same comments apply to this part as to Part 1.

Comment 3

[Philipp von Weitershausen [:philikon]]

This is ok so long as we do what's discussed in bug 641074 comment 31. In the long run we want to do bug 641675.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 519235 [details] [diff] [review]
Part 1. v1

Hmm, these were never flagged for review. Seems to have become important...

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 519235 [details] [diff] [review]
Part 1. v1

>+    if (optionsRef.value.resultType != optionsRef.value.RESULTS_AS_TAG_CONTENTS) {
>+      // We only upload records which refer solely to folders by GUID.
>+      // They don't need any kind of processing, so just return.
>+      for each (let q in queriesRef.value) {
>+        for each (let f in q.getFolders()) {
>+          if (f.length != 12)     // GUIDs are 12 chars.
>+            return false;
>+        }
>+      }
>+      return true;
>+    }

I might be blind, but I can't find anywhere in bookmarks.js that we actually rewrite folder=PLACE_ID queries to folder=GUID. I also can't find evidence in Places that it understands folder=GUID queries. So not only would this code never really be run, it would also create invalid places, right?

Comment 6

[Richard Newman [:rnewman]]

(In reply to comment #5)

> I might be blind, but I can't find anywhere in bookmarks.js that we actually
> rewrite folder=PLACE_ID queries to folder=GUID. I also can't find evidence
> in Places that it understands folder=GUID queries. So not only would this
> code never really be run, it would also create invalid places, right?

We don't do this *yet*.

One of the proposed solutions to this problem (said problem being "all hell breaks loose when we sync folder ID queries") is for Places to migrate to using GUIDs for those references instead. I summarized the IRC discussions here:

  https://bugzilla.mozilla.org/show_bug.cgi?id=641074#c31

Firefox 6 will include the stopgap solution, which is for Sync to not die in a fireball when it encounters one of these records.

What this patch does is try to keep us out of that situation in the first place: refuse to apply bookmark queries that refer to folders by local Places IDs. 

That means that *right now* it would refuse to sync *any* folder queries (which is much safer than what do now now). If Places starts using GUIDs at some point, and thus incoming records refer to GUIDs, then it will transparently start applying those records. If they come up with a different scheme, this code can be discarded.

It doesn't create invalid places: the non-tag branch in this function doesn't modify the record. It simply chooses whether or not to apply it.

I'm happy to omit the innermost 'if', but we would need to make a change to Sync in parallel with whatever fix the Places team come up with.

Comment 7

[Marco Bonardo [:mak]]

yes, we should move to folder=guid, asap, this means we also need to fix the json and html backup to include guids.
I added this to Sync needs in the Places plan page, unfortunatly I don't feel like I have the time to fix that for FF6.

Comment 8

[Marco Bonardo [:mak]]

also, changing queries would mean killing results in any FF version < 6 due to bug 641074 :(

Comment 9

[Marco Bonardo [:mak]]

Actually, if we'd stop using folder=id and add a folderGuid=guid, how would that work for Sync (And related to this change)? IIRC Places ignores unknown query keys, so it would maybe be a more downgradable solution.

Comment 10

[Richard Newman [:rnewman]]

(In reply to comment #9)
> Actually, if we'd stop using folder=id and add a folderGuid=guid, how would
> that work for Sync (And related to this change)? IIRC Places ignores unknown
> query keys, so it would maybe be a more downgradable solution.

Any solution that Sync can:

1. Pass safely on to Places without errors being thrown inside innocuous operations, and 
2. Decide whether to pass through without heuristics

works for me. If earlier versions will quietly accept folderGuid queries, and we can refuse to apply records that include folder= without an accompanying folderGuid=, then that would seem to do the trick.

In that case, the "!= 12" check in this patch becomes "q.getFolderGUIDs().length == 0".

Comment 11

[Marco Bonardo [:mak]]

That stuff needs deep testing and checks in old versions, not sure I can figure it out soonish.
Btw q.getFolderGUIDs() can't work since the method would not exist in old versions, at that point you'd better parse the query url with a regex and extract any /folder[^=]*=([^&]+)/

Comment 12

[Richard Newman [:rnewman]]

(In reply to comment #11)
> That stuff needs deep testing and checks in old versions, not sure I can
> figure it out soonish.

Understood.

> Btw q.getFolderGUIDs() can't work since the method would not exist in old
> versions, at that point you'd better parse the query url with a regex and
> extract any /folder[^=]*=([^&]+)/

Well, we'd only be able to add this robust checking to new versions, so that's OK. We can't fix released code.

The only thing I'm trying to do is make sure that we don't add data that breaks Places. What we'd do in this instance is only add records that refer to folders by GUID. Old clients will continue to misbehave, but that's not something we can help.

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 519235 [details] [diff] [review]
Part 1. v1

Clearly review, since comment 5/comment 6 haven't been addressed in Places yet. Please renominate if/when this patch become relevant.

Comment 14

[Richard Newman [:rnewman]]

mak, did we switch to folder=guid yet?

Comment 15

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #14)
> mak, did we switch to folder=guid yet?

nope, we still have to do. I think there's a bug somewhere but I can't find it. Unless it's this one... we need one filed in Toolkit/Places. In Jan I'll do some retriaging and prioritization.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #15)

> nope, we still have to do. I think there's a bug somewhere but I can't find
> it. Unless it's this one... we need one filed in Toolkit/Places. In Jan I'll
> do some retriaging and prioritization.

Filed Bug 824502. Probably this one can be closed as soon as that work is done, but first I'd want to add some tests in Sync!

Comment 17

[Mark Banner (:standard8)]

I believe this bug is probably a non-issue now that bug 824502 is resolved?

Comment 18

[Lina Butler (ex-Mozilla)]

Yes. :-) Thanks!

Comment 19

[Richard Newman [:rnewman]]

More than that, we can't ever drop bookmarks on the floor just because they're inconvenient! I should never have written this patch :D