Closed Bug 1373610 Opened 3 years ago Closed 2 years ago

Make BookmarkJSONUtils.importFromJSON more robust for invalid/unknown bookmark entries

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

From bug 1095426, currently translateTreeTypes will throw if it finds a type that it doesn't understand. This will cause the whole import to be aborted.

We should improve this, possibly by throwing and remove the node in the caller code (that would require changing from for..of to indexing and fixing the looping index).
Priority: -- → P3
Blocks: 1095427
In bug 1404631 I'll make insertTree a bit more lenient, then we can see if there's more work to do on the importers side here.
Depends on: 1404631
No longer blocks: 1095427
Looks like the code is just invoking reportError, not throwing. Apart from removing the comment here http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/toolkit/components/places/BookmarkJSONUtils.jsm#445 what's left to do?
Flags: needinfo?(standard8)
(In reply to Marco Bonardo [::mak] from comment #2)
> Looks like the code is just invoking reportError, not throwing.

That's correct, however, if I'm reading it write, it will still continue processing the node. When it then hits insertTree, the node type would presumably be invalid, and insertTree would throw.

However, I think your recent changes in bug 1404631, would mean we'll just skip the node.

> Apart from
> removing the comment here
> http://searchfox.org/mozilla-central/rev/
> 40e8eb46609dcb8780764774ec550afff1eed3a5/toolkit/components/places/
> BookmarkJSONUtils.jsm#445 what's left to do?

We might just want to change it to something like "this is an invalid node that will be skipped by insertTree", or find some way of not processing it?
Flags: needinfo?(standard8)
Priority: P3 → P2
Assignee: nobody → standard8
Comment on attachment 8930546 [details]
Bug 1373610 - Clarify BookmarkJSONUtils' handling of invalid bookmark types and add a test.

https://reviewboard.mozilla.org/r/201676/#review207450

thanks
Attachment #8930546 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9bbfb2a7e42
Clarify BookmarkJSONUtils' handling of invalid bookmark types and add a test. r=mak
https://hg.mozilla.org/mozilla-central/rev/d9bbfb2a7e42
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.