This bug was filed from the Socorro interface and is report bp-dd070bde-c7a8-41f0-86f2-334912161017. ============================================================= It looks like during painting, we can end up in gfxFont::Draw, which calls gfxSVGGlyphsDocument::ParseDocument, which calls the XML parser, which calls nsContentSink::NotifyDocElementCreated, and some of the observers of that can be implemented in JS.

The document-element-inserted notification is typically used when people want to inject stuff into a document after it's been created, so we should be able to dispatch that asynchronously, I think.

Otherwise we can run the risk of running JS code during painting.

I haven't tested this on try yet due to tree closure. If this turns out to break tests, I'll fix them. Hopefully it won't be too bad.

Comment on attachment 8803538 [details] [diff] [review] Notify about the document element being created asynchronously Review of attachment 8803538 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty concerned about this. We use this notification in WebExtensions to inject our browser.* APIs into the script global for WebExt pages. If we do this asynchronously, it might be too late. I think a bunch of add-ons do similar things. I've noticed that many of the crazier crashes we get involve SVG. Here's another example: https://crash-stats.mozilla.com/report/index/37c85a89-4c38-4d1c-b9e6-ba99f2161014 Notice frame 27, where it looks like we're doing full-blown reflow. I don't think we have many options in this sort of situation. Some ideas: - During these paints, don't re-rasterize SVG images. I think maybe we could break out if this call fails: http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/image/VectorImage.cpp#870 This would mean that we wouldn't draw certain SVGs (presumably the ones we've never had to draw before). - Somehow bail out of these special paints if we hit SVG (i.e., fall back to the spinner). I'm not really sure how common SVG images are, or how often we don't have a pre-rasterized copy. Do you know Ehsan?

(In reply to Bill McCloskey (:billm) from comment #4) > Comment on attachment 8803538 [details] [diff] [review] > Notify about the document element being created asynchronously > > Review of attachment 8803538 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm pretty concerned about this. We use this notification in WebExtensions > to inject our browser.* APIs into the script global for WebExt pages. If we > do this asynchronously, it might be too late. I think a bunch of add-ons do > similar things. Sigh. > I've noticed that many of the crazier crashes we get involve SVG. Here's > another example: > https://crash-stats.mozilla.com/report/index/37c85a89-4c38-4d1c-b9e6- > ba99f2161014 > Notice frame 27, where it looks like we're doing full-blown reflow. I don't > think we have many options in this sort of situation. Some ideas: > - During these paints, don't re-rasterize SVG images. I think maybe we could > break out if this call fails: > http://searchfox.org/mozilla-central/rev/ > 84075be5067b68dc6cb3b89f999645650e68c05b/image/VectorImage.cpp#870 > This would mean that we wouldn't draw certain SVGs (presumably the ones > we've never had to draw before). Let's ask dholbert since he knows about SVG images. > - Somehow bail out of these special paints if we hit SVG (i.e., fall back to > the spinner). > > I'm not really sure how common SVG images are, or how often we don't have a > pre-rasterized copy. Do you know Ehsan? Sadly I think SVG images are supported in all browsers, and are pretty heavily used. Chrome has some telemetry on this: <https://www.chromestatus.com/metrics/feature/popularity#SVGSVGElementInDocument> which is at 12% of pageloads! (This is where the counter is captured: <https://chromium.googlesource.com/chromium/src/+blame/8a96cca441f37f144074b24e713f75ca553a1c20/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp#535>) (Fun little fact, did you know that the NYTimes logo is an SVG image?)

I also meant to add that even though SVG is commonly used, going through the code path that you've suggested may be less common so perhaps we can get away with this...

<dholbert> billm, hi! so in https://bugzilla.mozilla.org/show_bug.cgi?id=1311843#c4 , you suggested "During these paints, don't re-rasterize SVG images." What are "these paints"? <billm> dholbert: the paints we do in the middle of JS, when switching tabs <dholbert> ah, gotcha <billm> dholbert: these are paints where we're not allowed to run JS code during painting <billm> dholbert: it seems like the SVG code tends to create its own little documents, and that can run a lot of JS <dholbert> billm, the internal SVG-image documents should not be able to run JS (except maybe for some internall stuff, not sure) <billm> dholbert: chrome JS can still see all sorts of stuff happening though <billm> dholbert: like the document-element-inserted observer <billm> dholbert: it looks like that doesn't happen though if we have already rasterized the SVG. do you know how likely that is? <dholbert> billm, if we've painted the SVG before, basically (and it hasn't been evicted from the cache) <dholbert> billm, so if this is the first time you're viewing the tab, then we won't have the SVG rasterized <billm> dholbert: how big is the cache? <dholbert> billm, or if a while has gone by and you've looked at a lot of other images in the meantime (and/or the SVG rasterization has large dimensions), then it wouldn't be cached <dholbert> billm, a few hundred MB, I think? I don't recall exactly <billm> dholbert: and that counts the size of all the surfaces in the cache? <dholbert> yeah <dholbert> the relevant cache here is the "surface cache", and IIRC there are some prefs that configure it <dholbert> billm, here's where it's configured: https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#4388 <dholbert> looks like it's 1GB <billm> cool, thanks <dholbert> billm, (though it's also capped at 1/4 of memory, if that's smaller)

Bad news: <dholbert> billm, looks like we also evict cached surfaces after they've been unused for 60 seconds, too (via nsExpirationTracker) <billm> dholbert: oh, dear, that's a problem <billm> dholbert: where does that happen? <dholbert> billm, https://dxr.mozilla.org/mozilla-central/source/image/SurfaceCache.cpp#952-955 is where we get the expiration time <dholbert> billm, and https://dxr.mozilla.org/mozilla-central/source/image/SurfaceCache.cpp#895-899 is where we get notified & remove

<dholbert> billm, (I think we could return DrawResult::TEMPORARY_ERROR to bail out cleanly from VectorImage::Draw) I doubt anyone actually cares about document-element-inserted for these SVG documents. We could probably run the notification asynchronously in that case. Maybe for all XML document in general. However, I still suspect that we'll have issues beyond this observer with SVGs.

(In reply to Bill McCloskey (:billm) from comment #9) > <dholbert> billm, (I think we could return DrawResult::TEMPORARY_ERROR to > bail out cleanly from VectorImage::Draw) > > I doubt anyone actually cares about document-element-inserted for these SVG > documents. We could probably run the notification asynchronously in that > case. Maybe for all XML document in general. However, I still suspect that > we'll have issues beyond this observer with SVGs. Maybe we should start doing that and watch out for other issues which we may find later?

Daniel, any chance you can take this please? Thanks!

Sure, though I probably won't be able to look in detail until early next week. (other bugs have been front-of-mind for the last week, and this Th/Fri are US holidays) Leaving needinfo open, and assigning to myself.

(In reply to :Ehsan Akhgari from comment #5) > (Fun little fact, did you know that the NYTimes logo is an SVG image?) Side note: based on the functions mentioned in comment 0 (gfxFont::Draw / gfxSVGGlyphsDocument::ParseDocument), this bug was originally about SVG-in-opentype fonts, not about SVG images. Though, they share a lot of the same backend, so maybe a single solution here will address both. Bug 1310335 might be related, too.

Just to be sure -- I think bug 1328423 (backing out painting during GC) means this bug is now a non-issue. Is that right?

(GC isn't mentioned in any comments on this bug, but from our lunch discussion, I think this bug's scenario was only a concern if it happens during GC.)

(In reply to Daniel Holbert [:dholbert] from comment #14) > Just to be sure -- I think bug 1328423 (backing out painting during GC) > means this bug is now a non-issue. Is that right? Correct.

OK, I'll unassign & resolve this as WORKSFORME then.