420411 - Writing JSON to a file triggers a 'legalRange' assertion

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Dan Mills [:thunder]]

Attachment: test case

The attached script serializes bookmarks to a json file.  It uses the nsIJSON interface, and triggers this assertion, along with a couple of places warnings:

###!!! ASSERTION: U+0080/U+0100 - U+FFFF data lost: 'legalRange', file /Users/thunder/sources/mozilla-trunk/mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 811

The error is *not* generated if the object is serialized via uneval().

It's highly likely my particular bookmarks are part of the problem; I will attach my places.sqlite as well.

Place them both in the cwd and run the js with xpcshell.

Comment 1

[Dan Mills [:thunder]]

Attachment: my bookmarks

Comment 2

[Dan Mills [:thunder]]

Could this be related to bug #420397 ?

Comment 3

[Dan Mills [:thunder]]

Here's an interesting tidbit:

I noticed that the weave sync algorithm was detecting changes in bookmarks that hadn't been changed.  Upon further inspection it seems those specific bookmarks are the ones that aren't being serialized correctly, and presumably trigger the error.  One example:

https://bugzilla.mozilla.org/show_bug.cgi?id=374518

If I go to bug 374518, bookmark it, then run a sync operation (which does something similar to attachment #1 [details] [diff] [review]), then look at the snapshot with emacs, I see:

"{4c544ab8-98c1-cf42-ab89-b13f09b339f1}539":{
  "parentGUID":"{4c544ab8-98c1-cf42-ab89-b13f09b339f1}464",
  "index":27,
  "type":"bookmark",
  "title":"Bug 374518  Provide platform support to enable syncing of Places datamodel objects to a remote server",
  "URI":"https://bugzilla.mozilla.org/show_bug.cgi?id=374518",
  "tags":[],
  "keyword":null}

I'm not sure how the title will come across in this comment, I just copied and pasted it from emacs and it has a little square in it.  In emacs, it looks like:

"title":"Bug 374518 ^S Provide platform support to enable syncing of Places datamodel objects to a remote server",

Comment 4

[Dan Mills [:thunder]]

Natively the places APIs return JS-proper (UCS-2) strings, which uneval'ed will show as e.g. \u2013 (for &endash;).  So writing to disk the uneval() output with nsIFileOutputStream will work fine even without conversion.

My test case code, however, uses nsIFileOutputStream, which does not attempt conversion.  And it uses nsIJSON.encode() (not encodeToStream), which means nsIJSON does not attempt to convert either.  This means that we end up with a string where, if we were to chop off the high byte of every UCS-2 character, we'd lose some info (as opposed to the eval() case).

Thus, the assertion makes complete sense, and this bug is simply invalid.  Using nsIConverterOutputStream and specifying UTF-8 as the desired output correctly encodes the output.

Comment 5

[Brendan Eich [:brendan]]

I didn't read the code, but where did type safety go? Do we freely chop 16 bits to 8 bits without warning?

/be

Comment 6

[Dan Mills [:thunder]]

There is a warning, the failed assertion:

###!!! ASSERTION: U+0080/U+0100 - U+FFFF data lost: 'legalRange', file
/Users/thunder/sources/mozilla-trunk/mozilla/js/src/xpconnect/src/xpcconvert.cpp,
line 811

And I just did a quick test, for a bugzilla title (includes &endash;) you end up with 0x13, the low byte of 0x2013.

However, that said, I don't really understand the value of having the nsIFileOutputStream.write() interface at all, specially scriptable.  It makes it easy to fall into the same trap I did IMO--why not deprecate it in favor of either an explicit conversion (a la nsIConverterOutputStream), or an interface with an implicit UTF-8 conversion?

Comment 7

[Dan Mills [:thunder]]

On the way home it dawned on me that:

* assertions are silent on non-debug builds
* the assertion is coming from xpconvert, not nsIFileOutputStream
* it really would not make sense for nsIFileOutputStream to assert something which can clearly fail at runtime depending on input.

Should I file a new bug to make nsIFileOutputStream throw an exception?

Comment 8

[Dan Mills [:thunder]]

Found an open bug on this: bug 415250.  See that bug for follow-up on this issue.