Use ManuallyDrop in the rust representation of style structs.

RESOLVED FIXED in Firefox 68

Status

()

defect
RESOLVED FIXED
2 months ago
Last month

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

This allows us to not run destructors of any members of nsStyle*, which in turn allows us to:

  • Remove the hack that replaced all nsStrings for nsStringReprs.
  • Remove ns{,C}StringRepr (followup)
  • Add members with destructors to the style structs (you see where I'm going :)).
Assignee

Comment 1

2 months ago

We destroy them manually, so it's the right thing to do.

This allows us to not run destructors of any members of nsStyle*, which in turn allows us to:

  • Remove the hack that replaced all nsStrings for nsStringReprs.
  • Remove ns{,C}StringRepr (followup)
  • Add members with destructors to the style structs (you see where I'm going :)).
Assignee

Updated

2 months ago
Blocks: 1550554

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

  • Add members with destructors to the style structs (you see where I'm going :)).

Can you say what your desired end state is here? That would help in reviewing the direction of these patches. I'm guessing it's towards using more and more Rust types for the style struct members, or perhaps defining the style structs entirely on the Rust side?

Flags: needinfo?(emilio)
Assignee

Comment 3

2 months ago

Yes. Generally this allows to have rust-defined slices and such in style structs (see bug 1550554 for an example where this allows us to avoid doing silly copies for box shadows and such and clean up a bunch of stuff).

But I think regardless of that, this patch is necessary. What we have right now is a real footgun (see the nsStringRepr stuff to avoid double-freeing strings in style structs on drop).

Defining eventually all the structs in rust would be great. For now I'm just trying to remove the most complex glue code as time and such permits.

Flags: needinfo?(emilio)

Comment 4

Last month
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d0e8bded97a
Use ManuallyDrop for style structs. r=jwatt

Comment 6

Last month
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21897fb59491
Use ManuallyDrop for style structs. r=jwatt
Assignee

Updated

Last month
Flags: needinfo?(emilio)

Comment 7

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.