1488300 - Loading of external resources referenced from CSS like elements from SVG document should block onload

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

Currently, reftest snapshot isn't block on external SVG resources like those referenced from filter or mask-image properties. This can cause difficulty of writing tests for these features (for example see bug 1486252 and the backouts), and may have to use hacks.

The hacks are considered fine for internal tests, but may cause more impediment for wpt.

I had a look at the code, and it seems that the snapshot in web-platform reftest is currently only blocked on onload event, but as far as I know, there are many other stuff that doesn't block onload, like CSS image loading. It's not clear how do we currently workaround that since that should be something fairly common in tests.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Joel, do you have idea how wpt reftest currently handle renderings that don't block onload? Is it possible to put extra blocker somewhere to make external resource dependency more reliably?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

So based on discussion on IRC, what we know now is that:
1. Loading of CSS images do block onload
2. Decoding of CSS images doesn't block onload, but it blocks readback / snapshot via sync decoding

It is a bit tricky for external resources. Maybe we should make it block onload as well.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

So the problem here is that, external resources loading does block onload, but it doesn't for those from CSS because those may not happen before onload.

Currently, loading of external resources for stuff like mask and filter is triggered by painting. For mask image, it is done this path:
* nsDisplayMask::BuildLayer invokes SVGObserverUtils::GetEffectProperties, which
* invokes GetOrCreateMaskProperty which creates nsSVGMaskProperty, which
* holds a list of nsSVGPaintingProperty, which inherits nsSVGIDRenderingObserver, which
* holds an instance of IDTracker, and the constructor invokes IDTracker::Reset, which
* invokes nsIDocument::RequestExternalResource to kick off loading of the external resource

Start loading something until painting sounds like way too late. We should kick off it from style resolving, like, in FinishStyle.

However, the problem is that, nsIDocument::RequestExternalResource wants to know what element triggers that request. This is rather complicated because in FinishStyle we don't have information about the element.

Actually, our current handling seems to be completely wrong. Tracking down the resource loading code, it seems to be using the provided element for various security-related stuff (getting the principal, etc.), however, via the path mentioned above, we are assuming that the originator of the loading is the element which has the style applied, which feels wrong. The originator should be the element where the style is from, shouldn't it? I don't know how much that matters.

Anyway, even follow our current behavior, we should probably trigger the resource loading from post traversal, or find a way to pass element into FinishStyle?

It's not totally clear to me what's the right solution here.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

There's a bunch of image-related code in nsIFrame::DidSetStyleContext. It'd be nice to use that instead of putting more image code somewhere else I'd think.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Oh, DidSetComputedStyle. That sounds like the right place to put this code in! Thanks.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1488300 - Kick off external SVG resource loading from DidSetComputedStyle.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 9006147 [details]
Bug 1488300 - Kick off external SVG resource loading from DidSetComputedStyle.

Daniel Holbert [:dholbert] has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68592cefee57
Kick off external SVG resource loading from DidSetComputedStyle. r=dholbert

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12902 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12902
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/425946106?utm_source=github_status&utm_medium=notification)

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Err... this is funny. The wpt PR fails because the updated test has unstable result on Firefox Nightly, but this patch itself means to fix such instability. Maybe we can have the CI rerun once this change gets into the nightly.

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/68592cefee57

Comment 13

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> Err... this is funny. The wpt PR fails because the updated test has unstable
> result on Firefox Nightly, but this patch itself means to fix such
> instability.

Did things stabilize? I don't see any annotations suggesting the test fails intermittently.

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Watt [:jwatt] from comment #14)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> > Err... this is funny. The wpt PR fails because the updated test has unstable
> > result on Firefox Nightly, but this patch itself means to fix such
> > instability.
> 
> Did things stabilize? I don't see any annotations suggesting the test fails
> intermittently.

Yes, it got stablized and the upstream PR has been merged as indicated in comment 13. I trigger the CI task for wpt to run again after this bug got into Nightly.