Assertion failure: !IsNonCanonicalizedNaN(d)

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: decoder, Assigned: luke)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
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));
  }
(Assignee)

Comment 2

8 years ago
Created attachment 495030 [details] [diff] [review]
patch

With the assert gone, SCTAG_FLOAT_MAX needs to be fixed; it only happened to deserialize NaN before because 0xfff00000 > 0x7fff80000.
Attachment #495030 - Flags: review?(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.
Attachment #495030 - Flags: review?(jorendorff)
(Assignee)

Comment 4

8 years ago
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 :)
(Assignee)

Comment 5

8 years ago
Created attachment 497558 [details] [diff] [review]
patch2
Attachment #495030 - Attachment is obsolete: true
Attachment #497558 - Flags: review?(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.
Attachment #497558 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
http://hg.mozilla.org/tracemonkey/rev/74eea2c10449
Assignee: general → lw
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/74eea2c10449
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 624080
(Reporter)

Updated

7 years ago
Blocks: 676763
You need to log in before you can comment on or make changes to this bug.