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

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P2)

Description

[Mark Banner (:standard8)]

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

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 3

[Mark Banner (: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?

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1373610 - Clarify BookmarkJSONUtils' handling of invalid bookmark types and add a test.

Review commit: https://reviewboard.mozilla.org/r/201676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201676/

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9bbfb2a7e42
Clarify BookmarkJSONUtils' handling of invalid bookmark types and add a test. r=mak

Comment 7

[Stefan Hindli [:stefan_hindli]]

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