1444329 - Don't use nsIScriptableUnicodeConverter to read bookmarks

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P1)

Description

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

BookmarkJSONUtils.jsm uses nsIScriptableUnicodeConverter twice: once to read a plain JSON file and another time to read a compressed JSON file.

For the plain case, Fetch with its built-in JSON parsing should work. For the compressed case, OS.File.read() returns an ArrayBuffer, so it can be decoded using TextDecoder instead of nsIScriptableUnicodeConverter.

Comment 1

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

BookmarkJSONUtils.jsm is the last caller of nsScriptableUnicodeConverter::ConvertFromByteArray, so nsScriptableUnicodeConverter::ConvertFromByteArray should be removed when the callers are removed.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks.

This also changes the way an empty file is handled when importing bookmarks - it will now throw with a syntax error.
The reason for the change is that using fetch's json converter, can't handle an empty file, and we can't easily adjust the data without reverting to JSON.parse manually.

This patch also makes nsBrowserGlue.js ignore errors of importing old bookmark backups, so that it can continue to try to recover, e.g. setup backups on idle again.

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

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray.

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

Comment 4

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 8983026 [details]
Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks.

https://reviewboard.mozilla.org/r/248886/#review255000

Thanks. r+ with the nits fixed.

::: toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js:32
(Diff revision 1)
>    // check to ensure not renamed to jsonlz4
>    Assert.equal(mostRecentBackupFile, backupFile);
>    // inspect contents and check if valid json
>    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>                          createInstance(Ci.nsIScriptableUnicodeConverter);
>    converter.charset = "UTF-8";

Looks like `converter` above should be removed.

::: toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js:63
(Diff revision 1)
>    let mostRecentBackupFile = await PlacesBackups.getMostRecentBackup();
>  
>    // Decode lz4 compressed file to json and check if json is valid
>    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>                          createInstance(Ci.nsIScriptableUnicodeConverter);
>    converter.charset = "UTF-8";

Here, too, `converter` above should be removed.

Comment 5

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Comment on attachment 8983027 [details]
Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray.

https://reviewboard.mozilla.org/r/248888/#review255002

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8983026 [details]
Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248886/diff/1-2/

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8983027 [details]
Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248888/diff/1-2/

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8983026 [details]
Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks.

https://reviewboard.mozilla.org/r/248886/#review255216

r=me on the part fixing this bug, please move the other part to a separate bug.

::: browser/components/nsBrowserGlue.js:1667
(Diff revision 2)
> -            source: PlacesUtils.bookmarks.SOURCES.RESTORE_ON_STARTUP,
> +              source: PlacesUtils.bookmarks.SOURCES.RESTORE_ON_STARTUP,
> -          });
> +            });
> -          importBookmarks = false;
> +            importBookmarks = false;
> +          } catch (ex) {
> +            Cu.reportError(ex);
> +          }

would you mind moving this to a separate bug? The problem is that we should set importBookmarks and restoreDefaultBookmarks to "meaningful" values based on this failure, basically not leave the user with a completely empty profile.
Considered that, I'd prefer tracking that change separately.

::: toolkit/components/places/BookmarkJSONUtils.jsm:224
(Diff revision 2)
> +  },
> +
> +  async import(rootNode) {
>      // Change to nodes[0].children as we don't import the root, and also filter
>      // out any obsolete "tagsFolder" sections.
> -    nodes = nodes[0].children.filter(node => !node.root || node.root != "tagsFolder");
> +    let nodes = rootNode.children.filter(node => !node.root || node.root != "tagsFolder");

could we just node.root !== "tagsFolder"?

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8983026 [details]
Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248886/diff/2-3/

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8983027 [details]
Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248888/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0821414fc6e
Stop using nsIScriptableUnicodeConverter when importing bookmarks. r=hsivonen,mak
https://hg.mozilla.org/integration/autoland/rev/22c2645c283c
Remove nsIScriptableUnicodeConverter::convertFromByteArray. r=hsivonen

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e0821414fc6e
https://hg.mozilla.org/mozilla-central/rev/22c2645c283c