Closed Bug 641617 Opened 10 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: rnewman, Unassigned)

References

Details

(Whiteboard: [has patch][sync:bookmarks])

Attachments

(2 files)

See Bug 641074.
Attached patch Part 1. v1Splinter Review
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.
Attached patch Part 2. v1Splinter Review
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.
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.
Whiteboard: [has patch]
Comment on attachment 519235 [details] [diff] [review]
Part 1. v1

Hmm, these were never flagged for review. Seems to have become important...
Attachment #519235 - Flags: review?(philipp)
Attachment #519237 - Flags: review?(philipp)
Whiteboard: [has patch] → [has patch][needs review 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?
(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.
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.
also, changing queries would mean killing results in any FF version < 6 due to bug 641074 :(
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.
(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".
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[^=]*=([^&]+)/
(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 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.
Attachment #519235 - Flags: review?(philipp)
Attachment #519237 - Flags: review?(philipp)
mak, did we switch to folder=guid yet?
Flags: needinfo?(mak77)
Whiteboard: [has patch][needs review philikon] → [has patch][sync:bookmarks]
(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.
Flags: needinfo?(mak77)
Depends on: 824502
(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!
Assignee: rnewman → nobody
Status: ASSIGNED → NEW
I believe this bug is probably a non-issue now that bug 824502 is resolved?
Flags: needinfo?(kit)
Yes. :-) Thanks!
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(kit)
Resolution: --- → WORKSFORME
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
Resolution: WORKSFORME → INVALID
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.