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

VERIFIED FIXED in Firefox 31

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Mardeg, Assigned: heycam)

Tracking

({testcase})

Trunk
mozilla31
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 verified)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8347707 [details]
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.
(Reporter)

Updated

4 years ago
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
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.
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
(Assignee)

Updated

4 years ago
OS: Windows XP → All
Hardware: x86 → All
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.)
Flags: needinfo?(dbaron)
(Assignee)

Comment 5

4 years ago
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.
Flags: needinfo?(dbaron)
(Assignee)

Updated

4 years ago
Depends on: 957833
Blocks: 957833
No longer depends on: 957833
(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.
Flags: needinfo?(dbaron) → needinfo?(khuey)
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.
Flags: needinfo?(khuey)
(Assignee)

Comment 8

4 years ago
Created attachment 8392680 [details] [diff] [review]
patch

This implements the comment 3 idea.
Assignee: nobody → cam
Attachment #8392680 - Flags: review?(dbaron)
(Assignee)

Comment 9

4 years ago
Created attachment 8392682 [details] [diff] [review]
patch (with code this time)
Attachment #8392680 - Attachment is obsolete: true
Attachment #8392680 - Flags: review?(dbaron)
Attachment #8392682 - Flags: review?(dbaron)
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
Attachment #8392682 - Flags: review?(dbaron) → review+
(Assignee)

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f3c059e9bc
https://hg.mozilla.org/mozilla-central/rev/31f3c059e9bc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Reporter)

Comment 13

4 years ago
Great work guys, verifying the testcase works perfectly in 31.0a1 (2014-03-19)
per comment 13
Status: RESOLVED → VERIFIED
status-firefox31: --- → verified
(Reporter)

Comment 15

3 years ago
Created attachment 8401310 [details]
Testcase (see URL field for added #2011 hash)

Updated the testcase for the new spec format
Attachment #8347707 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
(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.
Flags: in-testsuite+
Depends on: 1181907
You need to log in before you can comment on or make changes to this bug.