616454 - Assertion failure: !IsNonCanonicalizedNaN(d)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12
Build Identifier: 

js> deserialize(serialize(-'test'))
Assertion failure: !IsNonCanonicalizedNaN(d), at jsclone.cpp:289

Tested on tracemonkey tip (58210:25fd3451c0ae).

Reproducible: Always

Comment 1

[Luke Wagner [:luke]]

The offending double value is 0xfff8000000000000 which is a completely valid nan although not *the* canonical nan (that would be 0x7ff8000000000000).  In fact, based on bug 584653, we accept a whole space of safe nans.  A necessary and sufficient condition would be:

  bool IsUnsafeNan(double d) {
    jsval_layout l;
    l.asDouble = d;
    return !JSVAL_IS_DOUBLE(JSVAL_FROM_LAYOUT(l));
  }

Comment 2

[Luke Wagner [:luke]]

Attachment: patch

With the assert gone, SCTAG_FLOAT_MAX needs to be fixed; it only happened to deserialize NaN before because 0xfff00000 > 0x7fff80000.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 495030 [details] [diff] [review]
patch

The serialized data will be used in files (to store IndexedDB data). It shouldn't expose this implementation detail, which could conceivably change, for whatever reason, once Mac OS X 10.5 retires or if we change Value representation again.

So instead, let's canonicalize during serialization and check during deserialization.

> bool
> SCOutput::writeDouble(jsdouble d)
> {
>-    JS_ASSERT(!IsNonCanonicalizedNaN(d));
>+    JS_ASSERT(!IsUnsafeNan(d));
>     return write(ReinterpretDoubleAsUInt64(d));
> }

Great so far as the assertion goes, but let's JS_CANONICALIZE_NAN here.

With that, we don't really need the assertion here.

I think the reader should be strict about this, since it's an opportunity to detect when the input is complete garbage. SCInput::readDouble (or its callers) should require the input to be canonical (maybe by calling checkDouble) rather than canonicalizing it.

Comment 4

[Luke Wagner [:luke]]

Sounds good.  I assumed there had been some reason not to canonicalize everything; should have figured that nan-canonicalization isn't exactly a bottleneck in structured cloning :)

Comment 5

[Luke Wagner [:luke]]

Attachment: patch2

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 497558 [details] [diff] [review]
patch2

I still think this:

> I think the reader should be strict about this, since it's an opportunity to
> detect when the input is complete garbage. SCInput::readDouble (or its callers)
> should require the input to be canonical (maybe by calling checkDouble) rather
> than canonicalizing it.

but r=me either way.

Comment 7

[Luke Wagner [:luke]]

(In reply to comment #6)
> I still think this:

Oops, I lost context between reading the comment and writing the patch and forgot about that request.  I'll do that.

Comment 8

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/74eea2c10449

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/74eea2c10449