Closed Bug 1442587 Opened 2 years ago Closed Last year

JSObject::swap uses js::Vector<Value> which is documented as illegal

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: anba, Assigned: jorendorff)

Details

Attachments

(4 files, 5 obsolete files)

(In reply to André Bargull [:anba] from comment #0)
> https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/js/src/vm/JSObject.cpp#1707,1716

This is safe because we're in the scope of an AutoSuppressGC.  It wouldn't hurt to use an AutoObjectVector here - the rooting is unnecessary but the overhead is negligible and then we could enable that check.
(In reply to André Bargull [:anba] from comment #2)
The JSON parser one should be converted to use GCVector, which would also simplify JSONParserBase::trace().

The others look wrong at first glance, but I don't have time to investigate properly right now.
BTW, thanks for looking at this.
According to this, CGConstList is only actually used with number values:

https://searchfox.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#4414

Strings go through atomIndices instead; regexps though objectList, and so on.
Severity: normal → critical
Priority: -- → P1
Attachment #9005475 - Flags: review?(jwalden+bmo)
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #9005477 - Flags: review?(jwalden+bmo)
Attachment #9005478 - Flags: review?(jwalden+bmo)
Attached patch Part 4: Ban js::Vector<Value> (obsolete) — Splinter Review
Attachment #9005479 - Flags: review?(sphink)
Severity: critical → normal
Comment on attachment 9005479 [details] [diff] [review]
Part 4: Ban js::Vector<Value>

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

WFM
Attachment #9005479 - Flags: review?(sphink) → review+
Attachment #9005475 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 9005477 [details] [diff] [review]
Part 2: Avoid Vector<Value> in JSONParser

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

::: js/src/vm/JSONParser.cpp
@@ +48,5 @@
>      for (size_t i = 0; i < stack.length(); i++) {
> +        if (stack[i].state == FinishArrayElement)
> +            stack[i].elements().trace(trc);
> +        else
> +            stack[i].properties().trace(trc);

Yeesh, that's, like, actually the right way to do it!

We could also do

  for (auto& elem : stack) {
    if (elem.state == ...)
      elem.elements().trace(trc);
    else
      elem.properties().trace(trc);
  }

but it's not truly necessary.
Attachment #9005477 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 9005478 [details] [diff] [review]
Part 3: Avoid Vector<Value> in the front end

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8548,5 @@
>  {
>      MOZ_ASSERT(length() == array->length);
>  
>      for (unsigned i = 0; i < length(); i++)
> +        array->vector[i] = DoubleValue(list[i]);

I guess ConstArray should change to NumberArray storing doubles maybe in a followup or something?

::: js/src/vm/JSScript.cpp
@@ +2985,5 @@
>      if (!script->shareScriptData(cx))
>          return false;
>  
> +    if (bce->numberList.length() != 0)
> +        bce->numberList.finish(script->consts());

Should JSScript::consts be renamed too?
Attachment #9005478 - Flags: review?(jwalden+bmo) → review+
Attachment #9005475 - Attachment is obsolete: true
Attachment #9005477 - Attachment is obsolete: true
Attachment #9007253 - Flags: review+
Attachment #9007254 - Flags: review+
(In reply to Jeff Walden [:Waldo] from comment #12)
> I guess ConstArray should change to NumberArray storing doubles maybe in a
> followup or something?

Yeah, I didn't do it because I was not prepared to touch XDR for the relatively small type-iness benefit. Filed bug 1489534.
Attachment #9005478 - Attachment is obsolete: true
Attachment #9005479 - Attachment is obsolete: true
Attachment #9007257 - Flags: review+
Attachment #9007258 - Flags: review+
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88b838fa1b4
Part 1: Avoid Vector<Value> in JSObject::swap. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/60a28b2e2493
Part 2: Avoid Vector<Value> in JSONParser. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/217ce4f92eb9
Part 3: Avoid Vector<Value> in the front end. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7d80003930
Part 4: Ban js::Vector<Value>. r=sfink
Keywords: checkin-needed
Attachment #9007258 - Attachment is obsolete: true
Comment on attachment 9007674 [details] [diff] [review]
Part 4: Ban js::Vector<Value>

Missing #include. Carrying forward review.
Flags: needinfo?(jorendorff)
Attachment #9007674 - Flags: review+
Keywords: checkin-needed
Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88bf0327d46
Part 2: Avoid Vector<Value> in JSONParser. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2019c9a30e
Part 3: Avoid Vector<Value> in the front end. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f6698dcc04
Part 4: Ban js::Vector<Value>. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/38e4a9e68cc1
Part 1: Avoid Vector<Value> in JSObject::swap. r=jorendorff
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.