Closed Bug 1444329 Opened 2 years ago Closed Last year

Don't use nsIScriptableUnicodeConverter to read bookmarks

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: hsivonen, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

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.
Priority: -- → P2
Whiteboard: [fxsearch]
BookmarkJSONUtils.jsm is the last caller of nsScriptableUnicodeConverter::ConvertFromByteArray, so nsScriptableUnicodeConverter::ConvertFromByteArray should be removed when the callers are removed.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
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.
Attachment #8983026 - Flags: review?(hsivonen) → review+
Comment on attachment 8983027 [details]
Bug 1444329 - Remove nsIScriptableUnicodeConverter::convertFromByteArray.

https://reviewboard.mozilla.org/r/248888/#review255002
Attachment #8983027 - Flags: review?(hsivonen) → review+
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"?
Attachment #8983026 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e0821414fc6e
https://hg.mozilla.org/mozilla-central/rev/22c2645c283c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Priority: P2 → P1
No longer depends on: 1467046
You need to log in before you can comment on or make changes to this bug.