Closed Bug 1295582 Opened 8 years ago Closed 8 years ago

Sync bookmark validator should better handle "multipleParents" check

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

(Whiteboard: [data-integrity])

Attachments

(1 file)

Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans
58 bytes, text/x-review-board-request
markh
: review+
Details 
There are a couple problems with how the bookmark validator detects multipleParents.

1. Orphans (nodes with no parentid or with a parentid that points to a missing node) that show up as a child of a different node will be reported as having multiple parents.

2. The cases where a parentid points to something that exists but is not actually the parent. Currently this will come up as multipleParents, but should very likely show up as a specific error.

Also, the comment on multipleParents is very unclear, and should be specific about the fact that it's talking about the children array of the parents, since it is easily confused as it is written now.
Assignee: nobody → tchiovoloni
Priority: -- → P2
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review72244

Thanks, but I wouldn't mind understanding some of the logic better here.

::: services/sync/modules/bookmark_validator.js:243
(Diff revision 2)
>          }
> +      }
> -        idToRecord.set(record.id, record);
> +      idToRecord.set(record.id, record);
> +
> +      if (record.type === "folder" && !record.children) {
> +        // Only ever seen this in really broken cases... is it a bug we should report?

yeah, I think we should :) It might make sense to add a new category - something like "invalidRecord" which is, say, the "id" and a fieldName. We might even be able to merge childrenOnNonFolder with this (which would still list "children" as the invalid field), and maybe even missingIDs (in which case the "id" field would be listed). It's probably OK to not say what the exact problem is (meaning you would need to inspect the record to deduce why "children" is an issue) but IMO that's reasonable.

So maybe open a new bug along these lines and change this comment to reference that bug?

::: services/sync/modules/bookmark_validator.js:247
(Diff revision 2)
> +      if (record.type === "folder" && !record.children) {
> +        // Only ever seen this in really broken cases... is it a bug we should report?
> +        record.children = [];
>        }
> +
>        if (record.children && record.type !== "livemark") {

while this isn't part of this patch, it got me thinking... IIUC, we don't expect livemarks to have children on the server record, and mine don't. ISTM that we should report children on a livemark as a problem.

::: services/sync/modules/bookmark_validator.js:249
(Diff revision 2)
> +        record.children = [];
>        }
> +
>        if (record.children && record.type !== "livemark") {
>          if (record.type !== 'folder') {
>            problemData.childrenOnNonFolder.push(record.id);

it looks like we should |continue| out of the loop here?

::: services/sync/modules/bookmark_validator.js:332
(Diff revision 2)
>          continue;
>        }
>  
>        if (parent.type !== 'folder') {
>          problemData.parentNotFolder.push(record.id);
> +        if (!parent.children) {

Similarly to the above, it seems wrong to continue with the fiction that this is a folder - it seems better to  mark it as an orphan (ie, pretending the parent doesn't exist at all)?

::: services/sync/modules/bookmark_validator.js:372
(Diff revision 2)
> -      for (let ci = 0; ci < folder.children.length; ++ci) {
> -        let child = folder.children[ci];
> -        if (typeof child === 'string') {
> -          let childObject = idToRecord.get(child);
> +      folder.unfilteredChildren = folder.children;
> +      folder.children = [];
> +      for (let ci = 0; ci < folder.unfilteredChildren.length; ++ci) {
> +        let child = folder.unfilteredChildren[ci];
> +        let childObject;
> +        if (typeof child == "string") {

I don't understand this. Isn't folder.children guaranteed to be strings at this point?
Priority: P2 → P1
Whiteboard: [data-integrity]
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review72244

> while this isn't part of this patch, it got me thinking... IIUC, we don't expect livemarks to have children on the server record, and mine don't. ISTM that we should report children on a livemark as a problem.

This is due to some weirdness in the way the bookmark records are implemented: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#121-130

This probably deserves a comment though.

> Similarly to the above, it seems wrong to continue with the fiction that this is a folder - it seems better to  mark it as an orphan (ie, pretending the parent doesn't exist at all)?

I think the specific case I was handling here was the case where it was a deleted record placeholder (e.g. type === "item") -- although this is more explicitly handled in a couple lines. 

But I'm not sure I agree, I think we would want to see this information in e.g. aboutsync.

> I don't understand this. Isn't folder.children guaranteed to be strings at this point?

No, on line 350 we try to replace them. This is a bit dubious, admittedly.
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review72244

> This is due to some weirdness in the way the bookmark records are implemented: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#121-130
> 
> This probably deserves a comment though.

Yeah, a comment makes sense.

> I think the specific case I was handling here was the case where it was a deleted record placeholder (e.g. type === "item") -- although this is more explicitly handled in a couple lines. 
> 
> But I'm not sure I agree, I think we would want to see this information in e.g. aboutsync.

Yeah, makes sense

> No, on line 350 we try to replace them. This is a bit dubious, admittedly.

huh - so when is it going to be a string? ie, in which cases will those other lines not replace the items? It would be great if this could be cleaned up, or at least commented on.
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review72244

> huh - so when is it going to be a string? ie, in which cases will those other lines not replace the items? It would be great if this could be cleaned up, or at least commented on.

Basically, the first time (line 350ish) we're starting with the children, getting their parent (as indicated by parentid), and replacing their slot in the parent's children list with themselves (e.g. it had their ID, as a string, and now has the actual record).

Then the 2nd time (this code), we're starting with the parents, and finding the children, and filling them in. Any item here that's still a string, is an item where we didn't catch it the first time around. E.g. either the id is in multiple parents, or the ID is a deleted or missing child, so the checks we do later in this loop.

You can check the unfilteredChildren property in aboutsync to see examples of the problem -- e.g. in the bug 1297234 data some of the children of places are strings before this 2nd loop.
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review72244

> yeah, I think we should :) It might make sense to add a new category - something like "invalidRecord" which is, say, the "id" and a fieldName. We might even be able to merge childrenOnNonFolder with this (which would still list "children" as the invalid field), and maybe even missingIDs (in which case the "id" field would be listed). It's probably OK to not say what the exact problem is (meaning you would need to inspect the record to deduce why "children" is an issue) but IMO that's reasonable.
> 
> So maybe open a new bug along these lines and change this comment to reference that bug?

I'm not convinced that I wasn't imagining this case anymore (or, more likely, it was a validator or aboutsync bug that caused it to seem possible to begin with). It shouldn't be possible for folders to not have a children array for the same reason that livemark's children array is guaranteed. (Note that the later checks for children/childGUIDs on non-folders are still necessary for deleted item).
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review75304

I think this is fine, but it would be great if that new comment could be clarified.

::: services/sync/modules/bookmark_validator.js:242
(Diff revisions 2 - 3)
>            continue;
>          }
>        }
>        idToRecord.set(record.id, record);
>  
> -      if (record.type === "folder" && !record.children) {
> +      // Sadly, just looking at the children property of a { type: "livemark" } record

Can you please clarify this comment - at face value it appears you are looking at children for a livemark here and it could be avoided by checking the record.type first? Or, I suspect I'm just misreading the intent of that comment (ie, maybe we have, somewhat unavoidably, already looked at .children so by this point it does already exist?
Attachment #8784959 - Flags: review?(markh) → review+
Comment on attachment 8784959 [details]
Bug 1295582 - Fix sync bookmark validator bugs around missing parents and orphans

https://reviewboard.mozilla.org/r/74292/#review75304

> Can you please clarify this comment - at face value it appears you are looking at children for a livemark here and it could be avoided by checking the record.type first? Or, I suspect I'm just misreading the intent of that comment (ie, maybe we have, somewhat unavoidably, already looked at .children so by this point it does already exist?

How is this? I agree the logic here is a bit weird.  I think it would also miss a possible corruption where there was a "children" array on a livemark, but it wasn't empty.  I'm not sure if I think this could ever happen, but it sounds more likely than some of the things the validator checks for.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/924be2624797
Fix sync bookmark validator bugs around missing parents and orphans r=markh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/924be2624797
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: