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

Jump to bottom

Harden assert for creating atom from raw pointer #18302

Merged
merged 1 commit into from Aug 30, 2017
Merged

Harden assert for creating atom from raw pointer #18302

merged 1 commit into from Aug 30, 2017

Conversation

upsuper
Copy link
Contributor

@upsuper upsuper commented Aug 30, 2017 •
edited by SimonSapin

One of Stylo's common assertion turns out to be from having a null Atom (see Gecko 1385925 bug comment 7). It would be helpful if we can catch this kind of crash earlier via a release-assertion.

This may regress performance to some extent which I'm not sure. But we can probably have it there for diagnosis for now, and remove later.

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko_string_cache/mod.rs
  • @canaltinova: components/style/gecko_string_cache/mod.rs
  • @emilio: components/style/gecko_string_cache/mod.rs

@highfive
Copy link

warning Warning warning

  • 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 Aug 30, 2017
@upsuper
Copy link
Contributor Author

upsuper commented Aug 30, 2017

r? @bholley

The patch is updated to asserting only when constructing Atom.

@highfive highfive assigned bholley and unassigned emilio Aug 30, 2017
@upsuper
Copy link
Contributor Author

upsuper commented Aug 30, 2017

There are six places we do Atom( to construct an Atom: https://searchfox.org/mozilla-central/search?q=%5CbAtom%5C(&case=true&regexp=true&path=rs%24

We have three assertions here. One of the rest is Atom::from_static, which is only used in generated code, so presumably it should never have null pointer. The other two are from result of Gecko's Atomize, which presumably should never return null, otherwise we may have had other problems without Stylo.

@bholley
Copy link
Contributor

bholley commented Aug 30, 2017

@bors-servo delegate+

I think we can drop it from with(). Any persistence of the atom outside the scope of the callback would require a clone() call, and that will trigger the other assertion in from(). Avoiding it on the ::with call will avoid extra overhead on [1]. That code is hot, and while I'm doubtful that an extra null-check matters at that scale, it's easier to play it safe.

r=me with that

[1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/servo/components/style/gecko/snapshot_helpers.rs#54

@bors-servo
Copy link
Contributor

✌️ @upsuper can now approve this pull request

@upsuper
Copy link
Contributor Author

upsuper commented Aug 30, 2017

@bors-servo r=bholley

@bors-servo
Copy link
Contributor

📌 Commit 4f6752e has been approved by bholley

@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. labels Aug 30, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 4f6752e with merge 421e6d8...

bors-servo pushed a commit that referenced this pull request Aug 30, 2017
Auto merge of #18302 - upsuper:atom-assert, r=bholley
421e6d8 
Harden assert for creating atom from raw pointer

One of Stylo's common assertion turns out to be from having a null Atom (see [Gecko 1385925 bug comment 7](https://bugzilla.mozilla.org/show_bug.cgi?id=1385925#c7)). It would be helpful if we can catch this kind of crash earlier via a release-assertion.

This may regress performance to some extent which I'm not sure. But we can probably have it there for diagnosis for now, and remove later.

<!-- 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/18302)
<!-- Reviewable:end -->
@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 Aug 30, 2017
@upsuper
Copy link
Contributor Author

upsuper commented Aug 30, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

⚡ Previous build results for android, mac-dev-unit, windows-msvc-dev are reusable. Rebuilding only arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4...

@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: bholley
Pushing 421e6d8 to master...

@bors-servo bors-servo merged commit 4f6752e into servo:master Aug 30, 2017
@upsuper upsuper deleted the atom-assert branch August 30, 2017 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bholley bholley

Labels
S-tests-failed The changes caused existing tests to fail.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@upsuper @highfive @bholley @bors-servo @emilio