Fix unsafe reference hazards in DOM code, again

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments)

Created attachment 8728599 [details]
log3.txt

We still haven't turned on this part of the gc analysis report, so people keep sneaking them in.  See attached log from inbound today.
Created attachment 8728617 [details] [diff] [review]
Fix unsafe reference gc hazards people snuck into DOM code
Attachment #8728617 - Flags: review?(bkelly)
Comment on attachment 8728617 [details] [diff] [review]
Fix unsafe reference gc hazards people snuck into DOM code

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

::: dom/system/OSFileConstants.cpp
@@ +929,5 @@
>    if (!JS_SetProperty(cx, objSys, "bits", valBits)) {
>      return false;
>    }
>  
> +  const dom::ConstantSpec umask_cs[] = {

Does this have a rooting implication or is it just a drive-by edit?
Attachment #8728617 - Flags: review?(bkelly) → review+
> Does this have a rooting implication or is it just a drive-by edit?

It's mostly the latter.  I was trying to understand why the other ConstantSpec arrays we have are not showing up in the hazard analysis, and thought it might be because they're const, hence made the change.  But upon further reflection and discussion with the jsapi folks, it seems that the real reason is that they're file-static and the analysis doesn't handle that right (it's designed for things with function lifetimes).

We should probably just change this bit to stop using DefineConstants and explicitly JS_DefineProperty instead, to eliminate the hazard.  Let me just do that.
Created attachment 8729149 [details] [diff] [review]
Patch on top of the other one to nix the ConstantSpec entirely
Attachment #8729149 - Flags: review?(bkelly)
Attachment #8729149 - Flags: review?(bkelly) → review+

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/622ce61e7a3c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.