Skip to content

/ servo

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
Merged

stylo: Destroy static Variables struct on shutdown. #15629

merged 1 commit into from Feb 20, 2017

Conversation

@heycam
Copy link
Member

heycam commented Feb 18, 2017 •
edited by larsbergstrom

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

r? @emilio

This change is Reviewable
@highfive
Copy link

highfive commented Feb 18, 2017

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

highfive commented Feb 18, 2017

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!
@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
Member Author

heycam commented Feb 18, 2017

Yeah, maybe. Although this is only temporary until we get around to support CSS Variables properly.
@emilio
emilio approved these changes Feb 18, 2017
View changes
Copy link
Member

emilio left a comment

r=me with or without that nit.
components/style/properties/gecko.mako.rs Outdated

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

This comment has been minimized.

Sign in to view
@emilio

emilio Feb 18, 2017

Member

nit: You can use .take() instead, and remove the *data = None.
components/style/properties/gecko.mako.rs Outdated
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)

This comment has been minimized.

Sign in to view
@emilio

emilio Feb 18, 2017

Member

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

This comment has been minimized.

Sign in to view
@heycam

heycam Feb 19, 2017

Author Member

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

bors-servo commented Feb 18, 2017

☔ The latest upstream changes (presumably #15627) made this pull request unmergeable. Please resolve the merge conflicts.
@heycam heycam force-pushed the heycam:variables-leak branch 2 times, most recently from 62cc1b1 to 82a6060 Feb 19, 2017
@heycam
stylo: Destroy static Variables struct on shutdown.
Loading status checks…
58fcd51
@heycam heycam force-pushed the heycam:variables-leak branch from 82a6060 to 58fcd51 Feb 19, 2017
@heycam
Copy link
Member Author

heycam commented Feb 19, 2017

I got rid of the Mutex, just to avoid it potentially showing up in profiles. re-r? @emilio
@bholley
bholley approved these changes Feb 19, 2017
View changes
components/style/properties/gecko.mako.rs
};
}
// This is only accessed from the Gecko main thread.
static mut EMPTY_VARIABLES_STRUCT: Option<nsStyleVariables> = None;

This comment has been minimized.

Sign in to view
@bholley

bholley Feb 19, 2017

Contributor

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

bors-servo commented Feb 19, 2017

✌️ @heycam can now approve this pull request
@heycam
Copy link
Member Author

heycam commented Feb 20, 2017

@bors-servo r=bholley,emilio
@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2017

📌 Commit 58fcd51 has been approved by bholley,emilio
@highfive highfive assigned bholley and unassigned emilio Feb 20, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2017

⌛ Testing commit 58fcd51 with merge eec8823...
@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2017

💔 Test failed - linux-rel-wpt
@heycam
Copy link
Member Author

heycam commented Feb 20, 2017

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2017

⌛ Testing commit 58fcd51 with merge 58aa6ce...
bors-servo added a commit that referenced this pull request Feb 20, 2017
@bors-servo
Auto merge of #15629 - heycam:variables-leak, r=bholley,emilio
Loading status checks…
58aa6ce 
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 -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 20, 2017

☀️ 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
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bholley bholley

@emilio emilio

Assignees

@bholley bholley

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@heycam @highfive @bholley @emilio @bors-servo
You can’t perform that action at this time.