Closed Bug 1367523 Opened 7 years ago Closed 7 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: 7 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: