950436 - CSS variables of data: images in generated content not visible until the real string is used

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mardeg]

Attachment: Testcase (see URL field for added #2011 hash)

Only tested on XP with HWA off.
STR:
1. Click on one of the year links to show the thumbnail on the left as a background-image in ::after content, or
1a. Or hover the mouse on the line of a book to zoom the same thumbnail into view on the right as a direct ::after content url()
2. Untick the checkbox for "Use CSS Variables" to switch in the images from normal data: URI strings of the same images
3. Once you see the thumbnails, re-tick the checkbox to use CSS variables again, seeing they now are visible and remain so for the rest of the browser session.

I wasn't sure if I needed the exact same string in both the variable and the non-variable versions of each image, so for ease of testing I made the "Eloquent Javascript" thumbnail a JPG version in the CSS variable string, and a PNG version in the non-variable string. All the other images match between CSS variable and non-variable strings.

I don't know if initial non-visibility of the images is involved, but every time I made a more reduced testcase than this it wasn't always fully reproducible, while this one always is at least on my machine.

Comment 1

[Cameron McCormack (:heycam)]

My first thought is that it's related to kicking off image loading in here:

  https://hg.mozilla.org/mozilla-central/file/c049cb230d77/layout/style/nsCSSDataBlock.cpp#l107

although I thought we should get in here when we resolve variable references.

Comment 2

[Cameron McCormack (:heycam)]

I think the problem is:

* mozilla::css::ImageValue is responsible for cancelling an image load when that object disappears
* normally a Declaration object would have an nsCSSValue that references the ImageValue, then the
  nsCSSValue is copied into the stack allocated rule data in nsRuleNode::WalkRuleTree
* even though the stack allocated rule data disappears by the time we're in
  nsRuleNode::ComputeStyleBackground, we'd still have a reference to the ImageValue from the Declaration
* if background-image is set with a variable reference, the Declaration object does not contain an
  nsCSSValue referencing an ImageValue (it's a token stream), so when nsRuleNode::WalkRuleTree returns,
  our only reference to the ImageValue is dropped

Comment 3

[Cameron McCormack (:heycam)]

David, would it make sense to keep an additional reference to the ImageValue from the nsCSSValueTokenStream value on the Declaration?  We could check after resolving the variable reference whether it has any ImageValues in it (like TryToStartImageLoad does), and add them to a table on the nsCSSValueTokenStream that would be cleared if the token stream went away.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My gut feeling is that that seems a bit hacky.  It feels a little more sensible to me to have the things that currently hold references to imgIRequest in the style structs instead hold references to css::ImageValue.  Would that fix the problem?  Or would your approach somehow yield fewer unique css::ImageValue objects -- if so, maybe it's worth it?

(I also started on a patch at one point to convert the rest of the places in style structs that still hold URIs to instead hold css::URLValue; see https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/026ab6a48fc9/unapplied.pass-cssvalue-url-through which is probably quite bitrotted at this point.)

Comment 5

[Cameron McCormack (:heycam)]

Yes I think storing the css::ImageValue on the style struct would work.  Maybe it could end up not cancelling a request that we would currently?

What I don't really understand is the table of imgIRequest objects on the ImageValue.  It seems like it is storing one request per document that uses the style with the image reference in it?  How is it that multiple documents can be using the same ImageValue?  Are all the requests in the table for the same actual image URL?  What is the "canonical" request (the one with the nullptr key) used for?

I'm wondering whether it is necessary to store the document that is used to look up that table on the style struct as well, so that we can grab out the right imgIRequest object.  That's slightly annoying if so.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Yes I think storing the css::ImageValue on the style struct would work. 
> Maybe it could end up not cancelling a request that we would currently?
> 
> What I don't really understand is the table of imgIRequest objects on the
> ImageValue.  It seems like it is storing one request per document that uses
> the style with the image reference in it?  How is it that multiple documents
> can be using the same ImageValue?  Are all the requests in the table for the
> same actual image URL?  What is the "canonical" request (the one with the
> nullptr key) used for?

So it looks like that dates back to https://hg.mozilla.org/mozilla-central/rev/5c730c1f2274 from bug 697230.

Multiple documents can use the same ImageValue when we share style sheets between documents, which we currently do for XUL (where style sheets are stored on the prototype document, which gets cloned for each instance of that document), and which we ought to do (see bug 904836) more often.

I guess my idea is less good an idea after that change (which I'd forgotten about) -- before that change, the ImageValue probably had a better correspondence to what we wanted to keep alive.

Maybe Kyle has a better idea here?

> I'm wondering whether it is necessary to store the document that is used to
> look up that table on the style struct as well, so that we can grab out the
> right imgIRequest object.  That's slightly annoying if so.

Yeah, it is annoying.


Though I suppose your original idea of storing the ImageValue on the declaration somehow is better anyway, especially if it's done in a way that the same ImageValue object will persist across restyling.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Sorry for taking a while to get back to this.

The canonical request is used to clone all of the other requests from.  The request associated with a given document is canceled when that document is torn down, so if we don't have a request per-document bad things happen.  This is also why we have the canonical request, so we're sure that we have a live request to clone from.  We could probably rework this bug imagelib is not fun to play with.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

This implements the comment 3 idea.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: patch (with code this time)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8392682 [details] [diff] [review]
patch (with code this time)

> static void
> MapSinglePropertyInto(nsCSSProperty aProp,
>                       const nsCSSValue* aValue,
>                       nsCSSValue* aTarget,
>                       nsRuleData* aRuleData)
> {
>     NS_ABORT_IF_FALSE(aValue->GetUnit() != eCSSUnit_Null, "oops");
>+    nsCSSValueTokenStream* tokenStream = nullptr;

drop the " = nullptr;".  I assume that was just for testing.

>+        aTarget->GetUnit() == eCSSUnit_TokenStream ?
>+            aTarget->GetTokenStreamValue() :
>+            nullptr;

This requires some careful comments both here and in ResolveVariableReferences, since you're looking at the value that this function is *writing* to.  (It's probably worth asserting that aTarget->GetUnit() is either eCSSUnit_Null or eCSSUnit_TokenStream, since otherwise we shouldn't be here.  If that assertion fails, then it might be a sign that aTarget isn't necessarily clean and might contain an *old* token stream.

>     if (ShouldStartImageLoads(aRuleData, aProp)) {
>         nsIDocument* doc = aRuleData->mPresContext->Document();
>-        TryToStartImageLoad(*aValue, doc, aProp);
>+        TryToStartImageLoad(*aValue, doc, aProp, tokenStream);
>     }
>     *aTarget = *aValue;

r=dbaron with that

Comment 11

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=cc0619e634ab

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f3c059e9bc

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31f3c059e9bc

Comment 13

[Mardeg]

Great work guys, verifying the testcase works perfectly in 31.0a1 (2014-03-19)

Comment 14

[Paul Silaghi, QA [:pauly]]

per comment 13

Comment 15

[Mardeg]

Attachment: Testcase (see URL field for added #2011 hash)

Updated the testcase for the new spec format

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Wes Kocher (:KWierso) from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/31f3c059e9bc

I see here that this is also covered automatically.