1325075 - Value::isGCThing is a footgun

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Value::isGCThing and Value::isMarkable do almost the same thing, but isGCThing returns true for NullValue.

We could remove isGCThing/toGCThing and convert all consumers to isMarkable. After that we can move JSVAL_TAG_NULL before JSVAL_LOWER_INCL_TAG_OF_GCTHING_SET and probably rename isMarkable/isGCThing to isGCThing/toGCThing.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Get rid of isGCThing/toGCThing

This converts all isGCThing/toGCThing calls to isMarkable/toMarkablePointer or something else. Most callers actually wanted isMarkable/toMarkablePointer.

I renamed isGCThing to isGCThingOrNull and made it private, we'll remove it in a later patch.

r? baku for the DOM changes, especially XMLHttpRequestWorker.cpp we discussed on IRC.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Fix Value::toGCCellPtr

There are only 3 callers of toGCCellPtr, and all of them expect a non-null pointer.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Refactor Value.h

This moves NULL out of the "GCThing set" in Value.h, and removes the private isGCThingOrNull and toGCThingOrNull methods.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Rename isMarkable/toMarkablePointer

Now we can rename isMarkable/toMarkablePointer to isGCThing/toGCThing with no change in behavior.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Combined patch

Here's the combined patch. It's smaller but harder to review.

Also, if you prefer isMarkable/toMarkablePointer, I can just drop part 4.

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8821196 [details] [diff] [review]
Part 1 - Get rid of isGCThing/toGCThing

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

r+ for the DOM part

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8821201 [details] [diff] [review]
Part 4 - Rename isMarkable/toMarkablePointer

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

This all looks great!

Thanks for splitting up for easier reviewing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5eab325b27f
Fix Value::isGCThing footgun, stop returning true for NullValue. r=jonco,baku

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5eab325b27f