Closed Bug 1367523 Opened 8 years ago Closed 8 years ago

stylo: determine if clone of fontface and counter style rules create a memory leak, and re-enable layout/style/test/test_stylesheet_clone_font_face.html

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(3 files, 3 obsolete files)

Bug 1367523 Part 2: Gecko-side implement the CounterStyle and FontFaceRule clone functions.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1367523 Part 3: Re-enable layout/style/test/test_stylesheet_clone_font_face.html.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1367523 Part 4: Update the layout/reftests/stylesheet-cloning/counter-style-rule-clone.html test to use correct unicode literals.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1339629 implements deep cloning of css rules, but some uses appear to leak memory. Mochitest layout/style/test/test_stylesheet_clone_font_face.html triggers cloning of a fontface rule, and appears to leak.
Priority: -- → P2
Summary: stylo: determine if clone of fontface rules creates a real memory leak, and re-enable layout/style/test/test_stylesheet_clone_font_face.html → stylo: determine if clone of fontface and counter style rules create a memory leak, and re-enable layout/style/test/test_stylesheet_clone_font_face.html
Attachment #8874570 - Attachment is obsolete: true
Attachment #8874524 - Flags: review?(cam)
Attachment #8874525 - Flags: review?(cam)
Attachment #8874526 - Flags: review?(cam)
Attachment #8874527 - Flags: review?(cam)
Comment on attachment 8874524 [details] Bug 1367523 Part 1: Servo-side define and call Gecko CounterStyle and FontFaceRule clone functions. https://reviewboard.mozilla.org/r/145514/#review150512 ::: servo/components/style/stylesheets/mod.rs:291 (Diff revision 3) > let rule = arc.read_with(guard); > CssRule::Media(Arc::new( > lock.wrap(rule.deep_clone_with_lock(lock, guard)))) > }, > CssRule::FontFace(ref arc) => { > - let rule = arc.read_with(guard); > + let rule = arc.read_with(&guard); Why do we need this "&"? ::: servo/components/style/stylesheets/mod.rs:296 (Diff revision 3) > - let rule = arc.read_with(guard); > - CssRule::FontFace(Arc::new(lock.wrap(rule.clone()))) > + let rule = arc.read_with(&guard); > + CssRule::FontFace(Arc::new(lock.wrap( > + rule.deep_clone_from_gecko()))) > }, > CssRule::CounterStyle(ref arc) => { > - let rule = arc.read_with(guard); > + let rule = arc.read_with(&guard); ...and here?
Attachment #8874524 - Flags: review?(cam) → review+
Comment on attachment 8874525 [details] Bug 1367523 Part 2: Gecko-side implement the CounterStyle and FontFaceRule clone functions. https://reviewboard.mozilla.org/r/145516/#review150514
Attachment #8874525 - Flags: review?(cam) → review+
Comment on attachment 8874526 [details] Bug 1367523 Part 3: Re-enable layout/style/test/test_stylesheet_clone_font_face.html. https://reviewboard.mozilla.org/r/145892/#review150516
Attachment #8874526 - Flags: review?(cam) → review+
Comment on attachment 8874527 [details] Bug 1367523 Part 4: Update the layout/reftests/stylesheet-cloning/counter-style-rule-clone.html test to use correct unicode literals. https://reviewboard.mozilla.org/r/145894/#review150520 Is there a problem with the non-ASCII values?
Attachment #8874527 - Flags: review?(cam) → review+
Comment on attachment 8874527 [details] Bug 1367523 Part 4: Update the layout/reftests/stylesheet-cloning/counter-style-rule-clone.html test to use correct unicode literals. https://reviewboard.mozilla.org/r/145894/#review150520 Yes, strangely, setting the symbols property to a string that contains unicode characters doesn't "stick". I'll search for an existing bug and file one if necessary.
Attachment #8875512 - Flags: review?(cam)
Comment on attachment 8874527 [details] Bug 1367523 Part 4: Update the layout/reftests/stylesheet-cloning/counter-style-rule-clone.html test to use correct unicode literals. https://reviewboard.mozilla.org/r/145894/#review150520 Nevermind; I was using incorrect unicode literals -- correct for CSS, wrong for JS.
Comment on attachment 8875512 [details] Bug 1367523 Part 1b: Servo-side make cloning of FontFaceRule and CounterStyleRule compile for both servo and gecko configs. https://reviewboard.mozilla.org/r/146938/#review151056 ::: servo/components/style/stylesheets/mod.rs:293 (Diff revision 1) > - if cfg!(feature = "gecko") { > + rule.clone_conditionally_gecko_or_servo()))) > - rule.deep_clone_from_gecko() > - } else { > - rule.clone() > - }))) This change is fine. But FWIW you can do real conditional compilation on expression blocks, like this, if you wanted to keep the switching behaviour in here: CSSRule::FontFace(Arc::new(lock.wrap( #[cfg(feature = "gecko")] { rule.deep_clone_from_gecko() } #[cfg(not(feature = "gecko"))] { rule.clone() } )))
Attachment #8875512 - Flags: review?(cam) → review+
Comment on attachment 8875512 [details] Bug 1367523 Part 1b: Servo-side make cloning of FontFaceRule and CounterStyleRule compile for both servo and gecko configs. https://reviewboard.mozilla.org/r/146938/#review151374 ::: servo/components/style/stylesheets/mod.rs:293 (Diff revision 1) > - if cfg!(feature = "gecko") { > + rule.clone_conditionally_gecko_or_servo()))) > - rule.deep_clone_from_gecko() > - } else { > - rule.clone() > - }))) Rust compiler choked on that, not happy to find another # directive after the first block. I wasn't able to find a way to fight through it, so I'm keeping the proposed method.
Attachment #8874524 - Attachment is obsolete: true
Attachment #8875512 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f0ce0e021b87 Part 2: Gecko-side implement the CounterStyle and FontFaceRule clone functions. r=heycam https://hg.mozilla.org/integration/autoland/rev/2d26be0ac13f Part 3: Re-enable layout/style/test/test_stylesheet_clone_font_face.html. r=heycam https://hg.mozilla.org/integration/autoland/rev/30a798334757 Part 4: Update the layout/reftests/stylesheet-cloning/counter-style-rule-clone.html test to use correct unicode literals. r=heycam
Landed in mozilla-central: changeset: 363082:30a798334757 user: Brad Werth <bwerth@mozilla.com> date: Mon Jun 05 11:59:24 2017 -0700 summary: Bug 1367523 Part 4: Update the layout/reftests/stylesheet-cloning/counter-style-rule-clone.html test to use correct unicode literals. r=heycam changeset: 363081:2d26be0ac13f user: Brad Werth <bwerth@mozilla.com> date: Tue May 30 17:55:42 2017 -0700 summary: Bug 1367523 Part 3: Re-enable layout/style/test/test_stylesheet_clone_font_face.html. r=heycam changeset: 363080:f0ce0e021b87 user: Brad Werth <bwerth@mozilla.com> date: Mon May 22 17:21:09 2017 -0700 summary: Bug 1367523 Part 2: Gecko-side implement the CounterStyle and FontFaceRule clone functions. r=heycam
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: