Closed Bug 517224 Opened 15 years ago Closed 15 years ago

Firefox downloads CSS background images that it doesn't need (from overridden CSS rules)

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: alex, Assigned: dbaron)

References

(
URL
)

Details

(Keywords: perf)

Attachments

(1 file)

patch
3.03 KB, patch
bzbarsky
: review+
roc
: approval1.9.2+
Details | Diff | Splinter Review 
Summary: 
If a page has two CSS rules for a given element with each rule defining a different background image, Firefox will use the rule with higher specificity when it renders the page, but it will still download both background images.

I've created a test page that demonstrates this:
http://www.handcoding.com/documents/testing/redundant-image-download/

The page includes the following CSS:

div.photo
{
    background-image: url(morning-coffee.jpg);
    background-repeat: no-repeat;
}

    body.test-page div.photo
    {
        background-image: url(purple-horizon.jpg);
        background-repeat: no-repeat;
        width:  500px;
        height: 333px;
    }

Because the page's body element has the "test-page" class, the div.photo element later in the page should get the "purple-horizon.jpg" background image (and it does). However, if you load the page with Firebug's "Net" panel open, you can see that Firefox downloads both "morning-coffee.jpg" and "purple-horizon.jpg".

I'm running with this build:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090916 Minefield/3.7a1pre

(Feel free to move this to another component if it would be better suited elsewhere.)
Yeah, we start the download for any rule that matches (see the StartImageLoad calls in nsCSSDataBlock.cpp); we could probably delay further, although possibly at the cost of having to go to the network code for every element that uses the image rather than once per rule.
(In reply to comment #1)
> Yeah, we start the download for any rule that matches (see the StartImageLoad
> calls in nsCSSDataBlock.cpp); we could probably delay further, although
> possibly at the cost of having to go to the network code for every element that
> uses the image rather than once per rule.

One of the scenarios that comes to mind is one in which a developer may be using a JavaScript library that included a set of base CSS for its widgets (with the presumption that more advanced developers would override those base rules with their own CSS).

Given our limit of 6 HTTP connections per server, my concern is that extra background-image downloads could potentially fill up those slots and end up impeding the downloading/rendering of resources that are actually in-use by the page.
(In reply to comment #1)
> Yeah, we start the download for any rule that matches (see the StartImageLoad
> calls in nsCSSDataBlock.cpp); we could probably delay further, although
> possibly at the cost of having to go to the network code for every element that
> uses the image rather than once per rule.

Joe and Boris: Do you think dbaron's further-delay idea might be worth trying?
Trying, maybe.  It does have the issue David mentions: if multiple elements are using the background image then we'll possibly do a lot more work than now.  Which case is more common: that one, or the one you're worried about?  How big are the costs in both cases?  If the only concern is the http limit, does deprioritizing background image loads help?
Then again, maybe that wouldn't be a problem.  If we started the load from rulenode code when computing the nsStyleBackground, we should be ok, no?  After all, the ruledata just stores a pointer to the CSSValue::Image and thus would change the image in the original rule...
True, moving this from nsCSSDataBlock to nsRuleNode would be an insignificant delay in terms of getting loads started and would fix this.  It would also make nsCSSDataBlock cleaner, though it would also probably increase code size a little.
(In reply to comment #5)
> Then again, maybe that wouldn't be a problem.  If we started the load from
> rulenode code when computing the nsStyleBackground, we should be ok, no?  After
> all, the ruledata just stores a pointer to the CSSValue::Image and thus would
> change the image in the original rule...

Except, no, it wouldn't.  The ruledata copies the nsCSSValue object.

Maybe we should make nsRuleData always store pointers, though (and thus be different from nsCSS*).  That would also make them smaller, which would be good if we're going to keep them around.
> The ruledata copies the nsCSSValue object

Oh, hmm.  I thought we started the image loads from the Image itself, but you're right; we actually change the type of the nsCSSValue when we StartImageLoad.

Storing pointers in the nsRuleData would work, maybe, if we make HTML mapped attributes store nsCSSValues internally.  Or something...  Worried about ownership issues, and I'm not sure we want to start refcounting nsCSSValues and the like.
Going to imgLoader::LoadImage() once for every node that uses the image, rather than once per rule, won't be that bad. We reuse images pretty effectively, and don't expire images from the cache if they're in use. We won't touch the network multiple times.
> We reuse images pretty effectively

Sort of.  LoadImage() is not exactly cheap...  I've seen it come up a good bit in profiles.  It's not as expensive as a network load, but a few thousand calls to it during a pageload will slow you down nicely.
Actually... wait a sec; we can do exactly the equivalent in nsCSSDataBlock, and we already do for some of the cases.  We just don't for the ValueList case.
Attached patch patch — — Splinter Review 
The diff -w looks like this (because I added braces in two places while I was reindenting):

@@ -292,2 +292,5 @@
                 case eCSSType_ValueList:
+                case eCSSType_ValuePairList: {
+                    void** target = static_cast<void**>(prop);
+                    if (!*target) {
                     if (iProp == eCSSProperty_background_image ||
@@ -295,7 +298,8 @@
                         for (nsCSSValueList* l = ValueListAtCursor(cursor);
-                             l; l = l->mNext)
+                                 l; l = l->mNext) {
                             TryToStartImageLoad(l->mValue, doc);
+                            }
                     } else if (iProp == eCSSProperty_cursor) {
                         for (nsCSSValueList* l = ValueListAtCursor(cursor);
-                             l; l = l->mNext)
+                                 l; l = l->mNext) {
                             if (l->mValue.GetUnit() == eCSSUnit_Array) {
@@ -306,6 +310,4 @@
                     }
-                // fall through
-                case eCSSType_ValuePairList: {
-                    void** target = static_cast<void**>(prop);
-                    if (!*target) {
+                        }
+
                         void* val = PointerAtCursor(cursor);
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #412303 - Flags: review?(bzbarsky)
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
Also, to clean this code up a little, I wrote a patch now on bug 528634.
To write a test for this I probably need setSharedState
https://developer.mozilla.org/En/HTTP_server_for_unit_tests

I'm not going to do that now, though.
Attachment #412303 - Flags: review?(bzbarsky) → review+
Comment on attachment 412303 [details] [diff] [review]
patch

Oh, duh,  Good catch!
You think it's worth taking this on 1.9.2?
Well, it is a 1.9.2 regression as far as 'background-image' is concerned, though for the much-less-used 'content' and 'cursor' properties it's been present since bug 249809 / bug 38447.  And it's certainly safe enough.  So I guess I should request approval after I land...
Makes sense to me.
http://hg.mozilla.org/mozilla-central/rev/ff161ba309bd
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
VERIFIED FIXED with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091115 Minefield/3.7a1pre
Status: RESOLVED → VERIFIED
Attachment #412303 - Flags: approval1.9.2? → approval1.9.2+
Thanks to Waldo's advice, I now have a working mochitest for this bug (I confirmed that it fails when I make the current equivalent of undoing the patch and passes normally):

http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/0205a999790d/test-no-load-background-image

and I'll land it when I get the chance.
Assuming image caching works the way I hope/expect it would/should, you can avoid using shared state by having a single SJS file switch on the query string (so you'd have bug517224.sjs?image as doing the image stuff and bug517224.sjs?script as doing the script stuff, query being necessary to have the two requests cache differently).  This would avoid potential problems in the future if someone else ever used the "imageloaded" key with shared state in another test and didn't reset back to "" at test completion.

So far we've avoided using string-valued, shared-state functionality wherever possible.  I don't expect we'll be able to do this forever, but I think we can keep doing it as far as this bug is concerned.

You should use addLoadEvent or perhaps SimpleTest.executeSoon rather than setTimeout, I think.
It's probably also worth a comment (perhaps in the test's error message, I'm not sure what the etiquette has been for this historically) that this test will fail if the test is manually rerun (i.e. without re-executing runtests.py).
Oh, so the non-shared state is still shared between different requests for the same .sjs file.
Yes; more precisely, it's shared between requests for the same HTTP path, up to the ? delimiting path from query.  (It would be markedly less useful if the query string were included when segmenting private state space!)  I thought I'd made that clear in the MDC page, but a look back shows it really wasn't, so I modified it to explain that better.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: