stylo: whitelist moz-border-*-colors related bindings to satisfy heap write analysis

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
Attachment #8861340 - Flags: review?(cam)
Comment on attachment 8861340 [details]
Bug 1359313 - whitelist -moz-border-*-colors related bindings to satisfy heap write analysis.

https://reviewboard.mozilla.org/r/133330/#review136122
Attachment #8861340 - Flags: review?(cam) → review+
A quick fix for hazard bustage by increase the NUM_ALLOWED_WRITE_HAZARDS is pushed in bug 1348173 comment 37. In this bug, we shall do the actual fix and restore the NUM_ALLOWED_WRITE_HAZARDS.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e5a0162d61613dcae5002b7e68add2cfcda2f02
Looks like adding outparams whitelist is not enought, since we further pass nsStyleBorder struct to set/clear its members. I'll try adding nsStyleBorder to the class/struct whitelist and see if this could be a fix.
Comment hidden (mozreview-request)

Comment 6

10 months ago
We shouldn't be whitelisting entire classes, we should be whitelisting specific parameters as being writable.
Comment hidden (mozreview-request)
try looks good with the latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d97fb7a067eb9a91ce90ba122fcf82b634193c5f

Per Bobby's comment (comment 6), I'm not sure if this is the right fix, so request another round of review from Bobby.
Attachment #8861340 - Flags: review?(bobbyholley)

Comment 9

10 months ago
mozreview-review
Comment on attachment 8861340 [details]
Bug 1359313 - whitelist -moz-border-*-colors related bindings to satisfy heap write analysis.

https://reviewboard.mozilla.org/r/133330/#review136976

I would expect these to live in treatAsSafeArgument, where we would just whitelist the arg for writing, rather than whitelisting the function completely (which is dangerous).
Attachment #8861340 - Flags: review?(bobbyholley) → review-

Comment 10

10 months ago
mozreview-review
Comment on attachment 8861340 [details]
Bug 1359313 - whitelist -moz-border-*-colors related bindings to satisfy heap write analysis.

https://reviewboard.mozilla.org/r/133330/#review137140

Per IRC, move these up section so that the indirection explanation is clearer, and add some more detailed comments. r=me with that
Attachment #8861340 - Flags: review- → review+

Updated

10 months ago
Priority: -- → P1
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> Comment on attachment 8861340 [details]
> Bug 1359313 - whitelist -moz-border-*-colors related bindings to satisfy
> heap write analysis.
> 
> https://reviewboard.mozilla.org/r/133330/#review137140
> 
> Per IRC, move these up section so that the indirection explanation is
> clearer, and add some more detailed comments. r=me with that

Thank you for the review. :)

Since the patch is not modified (just moving couple lines up), so we shall reuse the try result in comment 4. I'm landing this.
Comment hidden (mozreview-request)

Comment 13

10 months ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7964174edf5f
whitelist -moz-border-*-colors related bindings to satisfy heap write analysis. r=bholley,heycam

Comment 14

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7964174edf5f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.