Closed Bug 1325075 Opened 3 years ago Closed 3 years ago

Value::isGCThing is a footgun

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(5 files)

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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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.
Attachment #8821196 - Flags: review?(jcoppeard)
Attachment #8821196 - Flags: review?(amarchesini)
There are only 3 callers of toGCCellPtr, and all of them expect a non-null pointer.
Attachment #8821197 - Flags: review?(jcoppeard)
This moves NULL out of the "GCThing set" in Value.h, and removes the private isGCThingOrNull and toGCThingOrNull methods.
Attachment #8821200 - Flags: review?(jcoppeard)
Now we can rename isMarkable/toMarkablePointer to isGCThing/toGCThing with no change in behavior.
Attachment #8821201 - Flags: review?(jcoppeard)
Attached patch Combined patchSplinter Review
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 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
Attachment #8821196 - Flags: review?(amarchesini) → review+
Attachment #8821196 - Flags: review?(jcoppeard) → review+
Attachment #8821197 - Flags: review?(jcoppeard) → review+
Attachment #8821200 - Flags: review?(jcoppeard) → review+
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.
Attachment #8821201 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5eab325b27f
Fix Value::isGCThing footgun, stop returning true for NullValue. r=jonco,baku
https://hg.mozilla.org/mozilla-central/rev/e5eab325b27f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.