Simplify BooleanGetPrimitiveValue

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: till, Assigned: till)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 761763 [details] [diff] [review]
Simplify BooleanGetPrimitiveValue

This is even simpler than the version I gisted earlier, in that it makes the entire call path infallible.

Given how much simpler this is than what we currently have, I don't think the fact that we have to know about the input being a CCW matters too much.
Attachment #761763 - Flags: review?(jwalden+bmo)
Comment on attachment 761763 [details] [diff] [review]
Simplify BooleanGetPrimitiveValue

Review of attachment 761763 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsbool.h
@@ +25,1 @@
>  BooleanGetPrimitiveValue(JSContext *cx, HandleObject obj, Value *vp);

Make this return the boolean value directly, rather than by outparam.  (And put the context at the end to indicate infallibility.)
Attachment #761763 - Flags: review?(jwalden+bmo) → review+
On my Linux box I get three regtest failures because of this:

  ecma_5/JSON/stringify-boxed-primitives.js
  ecma_5/JSON/stringify.js
  ecma_5/JSON/stringify-primitives.js

In all three cases the cause is the same:

  Assertion failure: wrappedBool->isCrossCompartmentWrapper(), at /home/njn/moz/mi8/js/src/jsbool.cpp:205
(Assignee)

Comment 5

5 years ago
Created attachment 764665 [details] [diff] [review]
Fix bustage caused by simplifying BooleanGetPrimitiveValue.

How did I not see this?
Attachment #764665 - Flags: review?(evilpies)
Comment on attachment 764665 [details] [diff] [review]
Fix bustage caused by simplifying BooleanGetPrimitiveValue.

Review of attachment 764665 [details] [diff] [review]:
-----------------------------------------------------------------

:)
Attachment #764665 - Flags: review?(evilpies) → review+
(Assignee)

Comment 7

5 years ago
So, this was actually backed out (https://hg.mozilla.org/integration/mozilla-inbound/rev/4703321564be), so I folded the two patches into one and relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c829d7d4e7
https://hg.mozilla.org/mozilla-central/rev/03c829d7d4e7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

5 years ago
Depends on: 886094
You need to log in before you can comment on or make changes to this bug.