EnsureNonSVGUserAgentStyleSheetsLoaded looks pretty bogus and slow for the IsBeingUsedAsImage() case.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments)

In the IsBeingUsedAsImage case, we pull from the category manager service and such, and start loading and parsing those as builtin "on-demand" UA sheets.

This has problems. As noted that needs to "append" to the UA sheet list. But right now we prepend and rely on loading it before other sheets.

However, nothing guarantees that we haven't loaded svg.css before (indeed we probably have), or any other sheet for that matter, so we're prepending those before the already-loaded ones. That looks bad.

Also, we're re-loading and reparsing the sheets even though nsStyleSheetService has a cached already-parsed copy of those. That's unnecessary work, cpu usage, and memory usage...
Actually, I wonder if that optimization is relevant anymore... This was done as part of bug 1008455 and similar followups. But that predates bug 77999, and the relevant UA stylesheet cache that we have:

  https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/servo/components/style/stylist.rs#65

I think we should try to remove it, that would fix, e.g., text-selection on Android, due to the accessible caret stuff...
I pushed the removal to awsy and talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cbd16216273eb9b2e48a250a98d8b02cf1613e8

So ni? myself to check it.
Flags: needinfo?(emilio)
Talos shows mostly noise on windows, nothing relevant otherwise. AWSY similarly nothing terrible, though it doesn't seem like the "image" test-cases were run, which are probably the interesting ones.

I forgot to repeat the talos runs, thus the noise, I'll try to push a better run, but I'm almost sure this is ok to remove nowadays :)
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Priority: -- → P3
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Talos shows mostly noise on windows, nothing relevant otherwise. AWSY
> similarly nothing terrible, though it doesn't seem like the "image"
> test-cases were run, which are probably the interesting ones.

Doesn't like in what way?  Was there a regression?  As you say, tests involving SVG images are probably relevant here...
Flags: needinfo?(emilio)
I didn't mention "doesn't like". I mentioned that there doesn't seem like they ran on the first place, but they did run on the push in comment 5, which shows no regression.
Flags: needinfo?(emilio)
Sorry, for some reason my brain skipped over the "seem" in your sentence, so I though I read that it doesn't like the tests!
Comment on attachment 8984799 [details]
Bug 1468133: Remove the optimization to lazily load non-SVG styles since it's not relevant anymore.

https://reviewboard.mozilla.org/r/250600/#review258064

Just to confirm, SVG as images will now load with a set of style sheets that will definitely use the same cached UA CasacadeData entry that we would get for a regular HTML document?  r=me if so.

::: commit-message-af2b3:14
(Diff revision 1)
> +This makes me wonder whether we could remove the whole concept of on-demand UA
> +sheets, since they've caused pain, for example, when the frontend people try
> +loading <svg>s from NAC (since that triggers sheet loading from frame
> +construction, which is not good). I'm not concerned about loading mathml.css and
> +svg.css everywhere, though xul.css may not be as doable since it adds a bunch of
> +attribute-dependent selectors. Though on the other hand I asserted in the
> +xul.css code and we don't load it in content with <video> / <input
> +type="date/time/etc"> and such, afaict, so maybe now that legacy addons are gone
> +we can remove that sheet from content processes altogether. Maybe worth at least
> +filing a followup?

Filing a followup with the contents of this paragraph sounds good.

::: dom/svg/SVGDocument.cpp
(Diff revision 1)
> -    EnsureOnDemandBuiltInUASheet(cache->NoFramesSheet());
> -  }
> -  if (nsLayoutUtils::ShouldUseNoScriptSheet(this)) {
> -    EnsureOnDemandBuiltInUASheet(cache->NoScriptSheet());
> -  }
> -  EnsureOnDemandBuiltInUASheet(cache->UASheet());

There is a comment in ua.css "SVG documents don't always load this file [...]".  Can you remove that?

And there are some rules that are duplicated from ua.css in svg.css.  Can you remove those?
Attachment #8984799 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #9)
> Comment on attachment 8984799 [details]
> Bug 1468133: Remove the optimization to lazily load non-SVG styles since
> it's not relevant anymore.
> 
> https://reviewboard.mozilla.org/r/250600/#review258064
> 
> Just to confirm, SVG as images will now load with a set of style sheets that
> will definitely use the same cached UA CasacadeData entry that we would get
> for a regular HTML document?  r=me if so.

I think that's right as of this patch, but I don't think that's absolutely guaranteed right now given media queries in svg.css. Though right now we don't load svg.css upfront anywhere, that's about to change because it blocks the frontend team (see bug 1469176).

Still I think this patch is worth taking as-is. I can analyze the UA cache in the content process and try to optimize for it (in particular, it's not clear to me that the media-query introduced in bug 686581 is terribly useful, see Dao's and Roc's discussion at the end), but I don't think it should block this. wdyt?
Flags: needinfo?(cam)
Verifying that this is all sharing properly sounds fine to me, thanks!
Flags: needinfo?(cam)
Blocks: 1470143
Blocks: 1470148
I filed bug 1470143 for the sharing stuff.
Blocks: 1470163
Posted patch fixup.patchSplinter Review
Had to do this as well to avoid calling nsPermissionManager early on when loading SVG images, since it was hitting some assertions. It effectively preserves the existing behavior, though I can also change AllowXULXBL if you prefer.
Attachment #8986774 - Flags: review?(cam)
Just to avoid a few assertions for AllowXULXBL now returning true, but LoadsXULStyleSheetUpFront being false. No behavior change. Before this bug the elements would be equally unstyled.

I can also just annotate the assertions if preferred, they're quite niche:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ddcc193db19d06aa0ab7c0e7027cb16a98be6b&selectedJob=184185578
Attachment #8986832 - Flags: review?(cam)
Attachment #8986774 - Flags: review?(cam) → review+
Attachment #8986832 - Flags: review?(cam) → review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8f6b544de1
Remove the optimization to lazily load non-SVG styles since it's not relevant anymore. r=heycam
From the commit message:
> The performance impact of having to lookup through more rules should
> be minimal given the bloom filter and the rule hash optimizations.

Fingers crossed. :)

> I asserted in the xul.css code and
> we don't load it in content with <video> /
> <input type="date/time/etc"> and such, afaict, so maybe now that
> legacy addons are gone we can remove that sheet from content processes
> altogether.

The purpose of minimal-xul.css is to have all the rules that HTML/SVG needs in order to function correctly (scrollbars, form controls, etc.). So we shouldn't need xul.css in the content process other than to continue supporting the pref dom.allow_XUL_XBL_for_file.
https://hg.mozilla.org/mozilla-central/rev/4c8f6b544de1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1496395
You need to log in before you can comment on or make changes to this bug.