Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stylo: Destroy static Variables struct on shutdown. #15629

Merged
merged 1 commit into from Feb 20, 2017

Conversation

heycam
Copy link
Contributor

@heycam heycam commented Feb 18, 2017

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs
  • @emilio: components/style/properties/gecko.mako.rs, ports/geckolib/glue.rs

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 18, 2017
@bholley
Copy link
Contributor

bholley commented Feb 18, 2017

Maybe this should be part of GlobalStyleData suggested in #15535? The mutex might be overkill.

@heycam
Copy link
Contributor Author

heycam commented Feb 18, 2017

Yeah, maybe. Although this is only temporary until we get around to support CSS Variables properly.

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with or without that nit.


pub fn shutdown() {
let mut data = EMPTY_VARIABLES_STRUCT.lock().unwrap();
if data.is_some() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You can use .take() instead, and remove the *data = None.

variables
}
};
static ref EMPTY_VARIABLES_STRUCT: Mutex<Option<nsStyleVariables>> = Mutex::new(None);
}

#[no_mangle]
#[allow(non_snake_case)]
pub unsafe extern "C" fn Servo_GetStyleVariables(_cv: ServoComputedValuesBorrowedOrNull)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd bet that we implemented this in C++? huh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to be, but we moved it over here when we had some linking issues in non-MOZ_STYLO builds.

@emilio
Copy link
Member

emilio commented Feb 18, 2017

I agree it'd be ok to make this more generic and not require a Mutex, but since it's only temporary I'd say that keeping CI green is more prioritary.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #15627) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Feb 18, 2017
@heycam heycam force-pushed the variables-leak branch 2 times, most recently from 62cc1b1 to 82a6060 Compare February 19, 2017 01:23
@heycam
Copy link
Contributor Author

heycam commented Feb 19, 2017

I got rid of the Mutex, just to avoid it potentially showing up in profiles. re-r? @emilio

};
}
// This is only accessed from the Gecko main thread.
static mut EMPTY_VARIABLES_STRUCT: Option<nsStyleVariables> = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this works because nsStyleVariables doesn't have a Drop impl, only the wrapper struct.

@bholley
Copy link
Contributor

bholley commented Feb 19, 2017

Still think we'll eventually want some unified system (GlobalStyleData) but this is fine for now.

@bors-servo delegate+

@bors-servo
Copy link
Contributor

✌️ @heycam can now approve this pull request

@heycam
Copy link
Contributor Author

heycam commented Feb 20, 2017

@bors-servo r=bholley,emilio

@bors-servo
Copy link
Contributor

📌 Commit 58fcd51 has been approved by bholley,emilio

@highfive highfive assigned bholley and unassigned emilio Feb 20, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Feb 20, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 58fcd51 with merge eec8823...

@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 20, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 58fcd51 with merge 58aa6ce...

bors-servo pushed a commit that referenced this pull request Feb 20, 2017
stylo: Destroy static Variables struct on shutdown.

Fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1340457.

r? @emilio

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15629)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 20, 2017
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: bholley,emilio
Pushing 58aa6ce to master...

@bors-servo bors-servo merged commit 58fcd51 into servo:master Feb 20, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants