Closed
Bug 1310335
Opened 8 years ago
Closed 6 years ago
nsCSSRuleProcessor::CascadeSheet() allocates a JS regexp object during InterruptCallback: MOZ_RELEASE_ASSERT(!isInsideUnsafeRegion()) ([AutoAssertNoGC] possible GC in GC-unsafe region)
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
WORKSFORME
mozilla53
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | + | fixed |
firefox53 | + | fixed |
People
(Reporter: jchen, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression, reproducible)
Crash Data
Attachments
(3 files)
6.12 KB,
patch
|
Details | Diff | Splinter Review | |
4.37 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-37c85a89-4c38-4d1c-b9e6-ba99f2161014. ============================================================= Crash due to the release assertion added in bug 1308039. Volume has been low so far, with 7 crashes on the 10-13 Nightly (#20 top crash) and 4 crashes on the 10-12 Nightly.
Reporter | ||
Comment 1•8 years ago
|
||
And that's on Windows. On Linux, the signature is slightly different, with 9 crashes on the 10-13 Nightly (#2 top crash) and 4 crashes on the 10-12 Nightly.
Crash Signature: [@ js::gc::GCRuntime::checkAllocatorState<T>] → [@ js::gc::GCRuntime::checkAllocatorState<T>]
[@ js::Allocate<T>]
Comment 2•8 years ago
|
||
We get a JS interrupt in order to paint, and then while restyling we hit a document rule that has a regex. This leads us to https://hg.mozilla.org/mozilla-central/annotate/f03e2740d604/dom/base/nsContentUtils.cpp#l6749 which allocates a JS regex object in order to evaluate it.
Blocks: 1308039
Comment 3•8 years ago
|
||
This is a chrome-only thing, apparently: https://dxr.mozilla.org/mozilla-central/source/layout/style/test/chrome/test_moz_document_rules.html
Updated•8 years ago
|
Component: JavaScript: GC → Layout
Updated•7 years ago
|
Crash Signature: [@ js::gc::GCRuntime::checkAllocatorState<T>]
[@ js::Allocate<T>] → [@ js::gc::GCRuntime::checkAllocatorState<T>]
[@ js::Allocate<T>] [@ JSObject::create] [@ JSObject* js::Allocate<T> ]
Updated•7 years ago
|
Summary: Crash in js::gc::GCRuntime::checkAllocatorState<T> → nsCSSRuleProcessor::CascadeSheet() allocates a JS regexp object during InterruptCallback
I have been getting this crash off late. If it helps, here are two recent crash reports: https://crash-stats.mozilla.com/report/index/b1b2bc37-177f-4fae-a039-17de42161126 and https://crash-stats.mozilla.com/report/index/4b6b7547-f30a-40f0-9b27-8b45b2161126
Comment 7•7 years ago
|
||
The trend in the crash volume looks a bit worrisome. David, can you think of a fix for this?
Would having a split version of nsContentUtils::IsPatternMatching where the caller could allocate the regexp object in advance (and store it in the DocumentRule::URL object) be sufficient to fix this? (i.e., is it only the allocation that's the problem?)
Flags: needinfo?(dbaron)
Setting flags based on where bug 1308039 landed, although I didn't double-check this based on crash-stats.
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
(Bug 1035091 might make this go away, but that carries significant compatibility risk, so you probably don't want to tie shipping bug 1308039 to it.)
This could use an assignee, and would probably make a good first project for an employee new to layout and/or dom (since it crosses them).
Flags: needinfo?(bugs)
Comment 12•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #8) > Would having a split version of nsContentUtils::IsPatternMatching where the > caller could allocate the regexp object in advance (and store it in the > DocumentRule::URL object) be sufficient to fix this? (i.e., is it only the > allocation that's the problem?) No, since JS_ExecuteRegExpNoStatics() also needs to allocate the match results object at the very least, and quite possibly does various other unsafe things too.
Comment 13•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #10) > (Bug 1035091 might make this go away, but that carries significant > compatibility risk, so you probably don't want to tie shipping bug 1308039 > to it.) Wouldn't userChrome.css and the like also possibly trigger this?
OK, that approach won't work then. The other thing I missed in the stack is that this goes through SVG-as-image codepaths. I think that makes the problem more general -- we're going from painting, to mozilla::image::VectorImage::CreateSurfaceAndShow, which decides to reflow and restyle the SVG image. (Note that restyling can trigger frame construction if there are style changes that require it.) It's not at all clear to me that this is the only problematic thing that would happen inside of that. Is the goal here to make it safe to *paint* inside of the interrupt callback -- or are we also committing to making it safe to restyle, construct frames, and reflow? (Are there things that run script inside of restyle, frame construction, or reflow codepaths that this would trigger?)
Flags: needinfo?(bugs)
Comment 15•7 years ago
|
||
Is it possible to separate the regexp logic and don't call into JS engine for this?
... though the stack in comment 0 goes through SVG-as-image, but the stacks in comment 6 take a different path to restyling, through this path: 15 xul.dll nsIDocument::FlushUserFontSet() dom/base/nsDocument.cpp:12652 16 xul.dll nsIDocument::GetUserFontSet() dom/base/nsDocument.cpp:12626 17 xul.dll nsLayoutUtils::GetFontMetricsForStyleContext(nsStyleContext*, float, unsigned char) layout/base/nsLayoutUtils.cpp:4329 18 xul.dll nsLayoutUtils::GetFontMetricsForFrame(nsIFrame const*, float) layout/base/nsLayoutUtils.cpp:4309 19 xul.dll nsLayoutUtils::GetInflatedFontMetricsForFrame(nsIFrame const*) obj-firefox/dist/include/nsLayoutUtils.h:1272 20 xul.dll mozilla::ScrollFrameHelper::GetLineScrollAmount() layout/generic/nsGfxScrollFrame.cpp:4091 21 xul.dll nsXULScrollFrame::GetLineScrollAmount() layout/generic/nsGfxScrollFrame.h:813 22 xul.dll nsLayoutUtils::ComputeScrollMetadata(nsIFrame*, nsIFrame*, nsIContent*, nsIFrame const*, mozilla::layers::Layer*, unsigned __int64, nsRect const&, mozilla::Maybe<nsRect> const&, bool, mozilla::ContainerLayerParameters const&) layout/base/nsLayoutUtils.cpp:8616 23 xul.dll nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) layout/base/nsDisplayList.cpp:1924
Comment 17•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #14) > OK, that approach won't work then. > > The other thing I missed in the stack is that this goes through SVG-as-image > codepaths. I think that makes the problem more general -- we're going from > painting, to mozilla::image::VectorImage::CreateSurfaceAndShow, which > decides to reflow and restyle the SVG image. (Note that restyling can > trigger frame construction if there are style changes that require it.) > It's not at all clear to me that this is the only problematic thing that > would happen inside of that. > > Is the goal here to make it safe to *paint* inside of the interrupt callback > -- or are we also committing to making it safe to restyle, construct frames, > and reflow? (Are there things that run script inside of restyle, frame > construction, or reflow codepaths that this would trigger?) See the discussion in bug 1311843 about the SVG image specific issue. I think Bill's intention was to only paint in this path. That being said, this particular issue can occur outside of the SVG image scenarios, as you already noted. What we definitely cannot do in this path is call back into SpiderMonkey. (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15) > Is it possible to separate the regexp logic and don't call into JS engine > for this? I think that's really the ultimate fix here. SpiderMonkey's regex code is really tied to SpiderMonkey and can't be used independently. And sadly, as far as I know, there are no other regex libraries in the tree. :( We could import something like https://github.com/google/re2 or https://doc.rust-lang.org/regex/regex/index.html even, but it's not clear to me how different the ES regex syntax is..
Comment 18•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-10 from comment #14) > I think that makes the problem more general -- we're going from > painting, to mozilla::image::VectorImage::CreateSurfaceAndShow, which > decides to reflow and restyle the SVG image. For context - in the design of SVG-as-an-image (and shoehorning it into our image APIs), this is analogous to "decoding" traditional raster images. IIRC when the svg-as-an-image code first landed, imagelib always automatically sent out a ready-to-paint notification as soon as all data had been received over the network, and then imagelib would flush decoding during paint if needed. And for SVG-an-an-iamge, that meant we needed to flush any unfinished layout/style during first paint. Imagelib's notifications have gotten a bit saner since then, though, so we could probably do this better now. In particular, I suspect/hope we could delay the ready-to-paint notification until we've finished restyle/reflow inside of the SVG image document -- and we could perhaps have a prompts from the outer document when it reflows the <img> (or the frame with this SVG background/border/bullet-image) to eagerly flush reflow at that point [so we can send the ready-to-paint notification], if needed.
Comment 19•7 years ago
|
||
Er, sorry -- it's more complicated than I was initially thinking in comment 18. I was thinking the SVG-image-reflow here was the initial reflow of the document -- but in comment 0's crash report (bp-37c85a89-4c38-4d1c-b9e6-ba99f2161014) it actually looks like it's a reflow from redrawing the image at a new size for the first time, which triggers us to resize the internal document's viewport. This triggers reflows (from reacting to the resize) and I think also can trigger restyles (from e.g. viewport-size-based media-query styles). I'd guess that some viewport-dependent media queries are involved in that crash report. This sort of reflow-during-painting isn't easy to prevent in general, particularly for images that are drawn at multiple sizes [whose size may change at paint-time]. We do cache rasterizations in imagelib's SurfaceCache, so a naive solution could be to "prime the cache" and do reflow/painting for known-interesting-sizes eagerly up-front. But those cached versions can expire [and might not fit in the cache in the first place]. So that probably won't work.
Updated•7 years ago
|
Comment 23•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] (PTO thru 12/18) from comment #19) > Er, sorry -- it's more complicated than I was initially thinking in comment > 18. > > I was thinking the SVG-image-reflow here was the initial reflow of the > document -- but in comment 0's crash report > (bp-37c85a89-4c38-4d1c-b9e6-ba99f2161014) it actually looks like it's a > reflow from redrawing the image at a new size for the first time, which > triggers us to resize the internal document's viewport. This triggers > reflows (from reacting to the resize) and I think also can trigger restyles > (from e.g. viewport-size-based media-query styles). I'd guess that some > viewport-dependent media queries are involved in that crash report. Can we detect the case where a reflow would be triggered in this case, and instead paint something (like a placeholder, or nothing) and post a runnable to actually do the reflow and paint asynchronously? Or am I oversimplifying things?
Flags: needinfo?(dholbert)
Comment 24•7 years ago
|
||
Can we do the regexp matching during parsing the CSS directly, like how we handle @support? I suppose parser is usually run in a stack which allows running js code?
Updated•7 years ago
|
Crash Signature: [@ js::gc::GCRuntime::checkAllocatorState<T>]
[@ js::Allocate<T>] [@ JSObject::create] [@ JSObject* js::Allocate<T> ] → [@ js::gc::GCRuntime::checkAllocatorState<T>]
[@ js::Allocate<T>] [@ JSObject::create] [@ JSObject* js::Allocate<T> ] [@ nsContentUtils::IsPatternMatching ]
Comment 25•7 years ago
|
||
For desktop Firefox we have this correlation: > (100.0% in signature vs 16.86% overall) Addon "Stylish" = true That might help with reproducing? Also, this bug is about this assertion failing: > MOZ_RELEASE_ASSERT(!isInsideUnsafeRegion()) ([AutoAssertNoGC] possible GC in GC-unsafe region) Unfortunately, because this is such a generic signature, there are a ton of Android crashes that match to this bug that don't have that assertion.
Summary: nsCSSRuleProcessor::CascadeSheet() allocates a JS regexp object during InterruptCallback → nsCSSRuleProcessor::CascadeSheet() allocates a JS regexp object during InterruptCallback: MOZ_RELEASE_ASSERT(!isInsideUnsafeRegion()) ([AutoAssertNoGC] possible GC in GC-unsafe region)
Comment 26•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #25) > Unfortunately, because this is such a generic signature, there are a ton of > Android crashes that match to this bug that don't have that assertion. Bug 1323207 made it so this particular crash should have a unique signature.
Updated•7 years ago
|
Version: unspecified → Trunk
Comment 27•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] (PTO until January 9th) from comment #25) > For desktop Firefox we have this correlation: > > > (100.0% in signature vs 16.86% overall) Addon "Stylish" = true > > That might help with reproducing? This clearly shows the @-moz-document rules in user stylesheets triggering the crash which is hardly surprising.
Comment 29•7 years ago
|
||
122 occurrences of this crash in the past week on Nightly, making it the #13 topcrash.
Comment 30•7 years ago
|
||
(In reply to :Ehsan Akhgari from comment #27) > (In reply to Nicholas Nethercote [:njn] (PTO until January 9th) from comment > #25) > > For desktop Firefox we have this correlation: > > > > > (100.0% in signature vs 16.86% overall) Addon "Stylish" = true > > > > That might help with reproducing? > > This clearly shows the @-moz-document rules in user stylesheets triggering > the crash which is hardly surprising. Stylish actually uses document level stylesheet, which means normal webpage can trigger this as well :/
Comment 31•7 years ago
|
||
Another solution I think would be always caching the result of @-moz-document matching. IIUC, the matching result can change only if: 1. the url changes, 2. the rule itself changes. Both cases should happen in a stack where js is allowed. If we cache the matching result and only update the cached value when necessary, this crash shouldn't be triggered anymore. Thoughts?
Comment 32•7 years ago
|
||
And it actually seems that @-moz-document rule currently doesn't respond to URL change at all (changing hash or using history.pushState doesn't trigger re-evaluation of the matching state, but restyle does reflect the change if triggered by something else), which makes me think that we can actually evaluate it at some very early stage, and use the result forever, as a upliftable quick fix. That shouldn't make anything worse.
Comment 33•7 years ago
|
||
And oh, the condition of @-moz-document is not mutable at all, which is another good news :)
Comment 34•7 years ago
|
||
I suggest that we have a bool member in document rule to record the value, which is initialized to false during parsing. Then we scan the stylesheet in CSSStyleSheet::SetOwningDocument, and evaluate every document rule and update the result there. After that, we always use that result. For optimization, we can have a flag in stylesheet to record whether we have any document rule inside. dbaron, do you think this would work?
Flags: needinfo?(dbaron)
That seems reasonable assuming something handles the initial evaluation correctly. (Is there always a SetOwningDocument call after parsing?)
Flags: needinfo?(dbaron)
Comment 36•7 years ago
|
||
Err, I guess my proposal in comment 34 may not work, at least style sheets added via nsDOMWindowUtils::AddSheet would never have mDocument set, and they are supposed to be shared between multiple documents. That means we cannot cache any document-specific thing inside the rule object. We need to find some different place to do that.
Comment 37•7 years ago
|
||
I have no idea how to reproduce this issue... I fail to reproduce it with a manual simple testcase which I guess should involve the crashing stack. And I also tried to put a gc-heavy page in a background tab, and I do see lots of GC happens, but it still doesn't crash. Is there any reliable STR for this?
Comment 38•7 years ago
|
||
I haven't tested this, but I've been assuming that STR would be something like this: (1) Have a -moz-document rule in a user stylesheet. (*hand-wave*) (2) View a page with an infinite JS loop in one tab. (Maybe even disable the slow-script dialogs, for good measure.) (3a) Change tabs from that infinite-loop-page to another tab that triggers evaluation of your -moz-document rule as part of its painting (*hand-wave*). ...or: (3b) Open a new tab and visit foo.com (where foo.com is the domain used in your -moz-document rule). IIUC, this should make us pause the infinite JS execution to paint the new foreground tab. And then we'd trigger this abort when evaluating the -moz-document rule as part of that paint. (Maybe this is what your manual testcase already approximately does, though?)
Flags: needinfo?(dholbert)
Comment 39•7 years ago
|
||
Bill is thinking of backing out bug 1308039 (in bug 1328423), so it might not be worth digging into this further until that is decided.
Comment 40•7 years ago
|
||
This is #5 for 20170109030209 build. It looks like we are going to back out bug 1308039 so I will block this one on bug 1328423.
Blocks: 1328423
Updated•7 years ago
|
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 41•7 years ago
|
||
I realized that there is actually a relatively easy way to fix this, by using the rust regex crate for the @-moz-document regex selectors. The regex syntax supported there <https://doc.rust-lang.org/regex/regex/index.html#syntax> is similar to the one supported by EcmaScript, the biggest notable difference that I can find is the lack of support for back references. I think that's a relatively fine trade-off to make here. I wrote a simple patch to do this and looks like it works on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b43a55852eefbc5f3c9044881aa19631f055a2b9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 42•7 years ago
|
||
Attachment #8848802 -
Flags: review?(michael)
Comment 43•7 years ago
|
||
Attachment #8848803 -
Flags: review?(michael)
Comment 44•7 years ago
|
||
The rust regex backend being used here doesn't support the exact same regex syntax as the EcmaScript regex syntax. The biggest difference is probably lack of support for back references. Given the fact that the @-moz-document regex selectors aren't a commonly used feature, and back references in such regexes are even a less commonly used feature, the trade-off of taking a regression in functionality like this in order to be able to paint in the middle of GCs seems to be worth it.
Attachment #8848804 -
Flags: review?(dbaron)
Comment 45•7 years ago
|
||
What would the codesize look like if we integrate another regex impl in it? IIRC regex impls usually have relatively large codesize. Also the syntax supported by regex may not match what is supported in JavaScript. Is it possible to use C++11's <regex>?
Comment 46•7 years ago
|
||
Comment on attachment 8848803 [details] [diff] [review] Part 2: Add support for using the rust regex backend to nsContentUtils::IsPatternMatching() Review of attachment 8848803 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.cpp @@ +6741,5 @@ > return nullptr; > } > > +extern "C" { > +bool rust_regex_match(const nsACString& aPattern, const nsACString& aValue); Please add a comment to this declaration with the location of the rust crate which implements the API.
Attachment #8848803 -
Flags: review?(michael) → review+
Comment 47•7 years ago
|
||
Comment on attachment 8848802 [details] [diff] [review] Part 1: Add a simple rust regex matcher function Review of attachment 8848802 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/rust-regex-match/.gitignore @@ +1,1 @@ > +/target Do we need an .hgignore as well? I don't we ever run cargo directly in this directory, but it would be nice to avoid having the artifacts checked into the tree if you're using hg. ::: dom/base/rust-regex-match/src/lib.rs @@ +9,5 @@ > +use regex::Regex; > +use nsstring::nsACString; > + > +#[derive(Debug)] > +enum RegexMatchError { Usually error types in rust implement ::std::fmt::Display and ::std::error::Error (Error requires Display) which means that they can be treated generically by code. @@ +26,5 @@ > + RegexMatchError::RegexError(err) > + } > +} > + > +fn regex_matcher(pattern: &nsACString, value: &nsACString) -> Result<bool, RegexMatchError> { Given that this isn't performance-sensitive code, it might be worthwhile to instead use a `Result<bool, Box<Error>>` (Error is ::std::error::Error - a trait which most errors in crates implement) which heap-allocates the error meaning that RegexMatchError is unnecessary. I imagine that the heap allocation would be optimized away anyway in this situation as the value in the error is never used. @@ +30,5 @@ > +fn regex_matcher(pattern: &nsACString, value: &nsACString) -> Result<bool, RegexMatchError> { > + // In the future we may consider doing something more efficient than > + // re-parsing the regex every single time, but this code is only used > + // in edge cases that shouldn't currently matter performance-wise. > + let re = try!(Regex::new(try!(str::from_utf8(pattern)))); Use ? instead of try!() - this would look like: let re = Regex::new(str::from_utf8(pattern)?)?; Ok(re.is_match(str::from_utf8(value)?))
Attachment #8848802 -
Flags: review?(michael)
Do we already import the Rust regex crate for something else?
Comment 49•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #48) > Do we already import the Rust regex crate for something else? No. I think we even intentionally disable the default feature of env_logger crate in stylo to avoid include regex. There are several reference of regex in our Cargo.lock, but they should be just build-dependencies.
Comment 50•7 years ago
|
||
See https://github.com/servo/servo/blob/a2c33ac6e6e18c186e9af45274a7b8499ba1bc4c/ports/geckolib/Cargo.toml#L19
Comment 51•7 years ago
|
||
It seems someone has started using C++11's <regex> in our codebase [1] since bug 1347679... Not sure whether that caused any bloat on code size, but I think that is at least a better direction than embedding Rust's regex crate at this moment, given that <regex>'s runtime can be provided by the system, but for Rust's regex we have to ship it with our binary. Also C++11's <regex> supports more JS regexp syntax [2], so we can regress fewer things. [1] https://dxr.mozilla.org/mozilla-central/rev/e1576dd8bd9d3a4ca418cf347133b8a4957ddeca/gfx/gl/GLContext.cpp#13 [2] http://en.cppreference.com/w/cpp/regex/ecmascript
Comment 52•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #51) > It seems someone has started using C++11's <regex> in our codebase [1] since > bug 1347679... Not sure whether that caused any bloat on code size, but I > think that is at least a better direction than embedding Rust's regex crate > at this moment, given that <regex>'s runtime can be provided by the system, > but for Rust's regex we have to ship it with our binary. This is false. The implementor of libstdc++'s <regex> told me they don't use explicit instantiation for it and thus the code size would go into our binary. See bug 1347679 comment 7.
Comment 53•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #52) > This is false. The implementor of libstdc++'s <regex> told me they don't use > explicit instantiation for it and thus the code size would go into our > binary. See bug 1347679 comment 7. If libstdc++'s <regex> had used explicit instantiation, patches using <regex> wouldn't even build on Linux, since we check for libstdc++ symbols newer than a certain version. In retrospect, I wish they *had* used explicit instantiation, as that seems like a much better choice in some respects...
Attachment #8848804 -
Flags: review?(dbaron) → review+
Comment 54•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #53) > If libstdc++'s <regex> had used explicit instantiation, patches using > <regex> wouldn't even build on Linux, since we check for libstdc++ symbols > newer than a certain version. In retrospect, I wish they *had* used > explicit instantiation, as that seems like a much better choice in some > respects... The implementer told me that they don't use explicit instantiation because the maintainer thought the impl may change over time, and using explicit instantiation to put them into runtime library may make it harder to keep back-compatibility. (It's a bit off-topic, though)
Comment 55•7 years ago
|
||
Sorry for the long lag here. Firstly, we can't use C++11 <regex> in our code base because it's not exception safe, I think there has been some confusion about this. I have commented in bug 1349064. This is why I went down the Rust path. I will do a couple of try pushes to get code size numbers on all platforms. But if the sizes end up being too high, do we have any other alternatives?
Blocks: 1347425
Comment 56•7 years ago
|
||
Before try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=942717b778d4f80425eaaf8d5846edbbfd9aa996 After try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a31d2b4422e51fcfdef39b6b082eb347f2eb36 Binary sizes: Linux64 PGO: from 100990904 to 101417944 (427040 larger) MacOSX: from 161496224 to 161962096 (465872 larger) Win32 PGO: from 52263936 to 52573696 (309760 larger) Win64 PGO: from 63697920 to 64017920 (320000 larger) Android API15+ APK: from 34922528 to 35017849 (95321 larger) (I didn't have a tool to extract the APK file here, but given the size difference between x86 and x86-64, I guess most of the bloat comes from some sort of data tables...) So, based on these measurements, these increases are way too high to be worth it for this bug... I think we should just WONTFIX this at this point, since for practical reasons we may never get to turn on painting during GC. If we ever want to do that and this actually becomes a blocker, *then* we can have an escape hatch! We can task someone with removing ~450KB dead B2G code. This isn't a day of work, but should be quite tractable, since we still have a lot of stuff that I suspect we can remove if someone worked on it. That way we would be able to take the code size hit from this addition with a clear conscious (but I'd rather remove that code and win that 450KB and never pay this cost TBH.) What do you think, Bill?
Flags: needinfo?(wmccloskey)
Although I agree that this isn't something we should work on right now (at least not without the APZ measurements we discussed), I think we could fix this in a different way. We would need to add an API to the JS engine that allows us to use irregexp without allocating JS objects. I'm guessing that would take a few days at most for jandem or bhackett. I looked into it a while ago and it's kind of a pain, but certainly tractable (I don't know the code nearly as well as they do).
Flags: needinfo?(wmccloskey)
Comment 58•7 years ago
|
||
That sounds like a good solution to me if it's doable!
Assignee: wmccloskey → nobody
Comment 59•7 years ago
|
||
Are these the same issue or should I open a separate bug(s) for that? https://crash-stats.mozilla.com/report/index/7d5a883a-447e-4c93-a2a1-d44fb0170624 https://crash-stats.mozilla.com/report/index/3eafe5ed-849b-4725-9eb6-6c2a50170624 Happens with layout.css.servo.enabled=true constantly :/
Comment 60•7 years ago
|
||
(In reply to Johannes Pfrang [:johnp] from comment #59) > Are these the same issue or should I open a separate bug(s) for that? > > https://crash-stats.mozilla.com/report/index/7d5a883a-447e-4c93-a2a1- > d44fb0170624 > > https://crash-stats.mozilla.com/report/index/3eafe5ed-849b-4725-9eb6- > 6c2a50170624 > > Happens with layout.css.servo.enabled=true constantly :/ These look like a different issue, since they're happening during cycle collection. Please file a new bug pointing at these crashes, and provide steps to reproduce the crash. Please also see if the steps to reproduce also reproduce the crash in a completely new profile. Thanks!
Comment 61•7 years ago
|
||
(In reply to Johannes Pfrang [:johnp] from comment #59) > Happens with layout.css.servo.enabled=true constantly :/ Oh, also, since this is with Stylo, please prefix your bug title with "stylo: ", so it's easier for us to find as a stylo problem. Thank you!
Comment 62•7 years ago
|
||
I'll remove some of these overly generic signatures.
Crash Signature: [@ js::gc::GCRuntime::checkAllocatorState<T>]
[@ js::Allocate<T>] [@ JSObject::create] [@ JSObject* js::Allocate<T> ] [@ nsContentUtils::IsPatternMatching ] → [@ nsContentUtils::IsPatternMatching ]
Comment 63•7 years ago
|
||
Filed bug 1376130 for #59 with STR.
Comment 64•6 years ago
|
||
This code is gone, and @-moz-document is no longer supported anyway.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•