Last Comment Bug 783162 - Background image isn't shown at all
: Background image isn't shown at all
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 4 votes (vote)
: mozilla17
Assigned To: Kyle Huey [:khuey] (
: 784481 (view as bug list)
Depends on: 786854 816498 817531
Blocks: 697230
  Show dependency treegraph
Reported: 2012-08-15 20:52 PDT by Toshihiro Yamada
Modified: 2013-02-07 08:44 PST (History)
22 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

html and background image for testing (1.37 KB, application/octet-stream)
2012-08-15 20:52 PDT, Toshihiro Yamada
no flags Details
Patch (26.71 KB, patch)
2012-08-17 16:00 PDT, Kyle Huey [:khuey] (
no flags Details | Diff | Splinter Review
Better patch (45.06 KB, patch)
2012-08-21 21:06 PDT, Kyle Huey [:khuey] (
bzbarsky: review+
Details | Diff | Splinter Review
Patch (59.89 KB, patch)
2012-08-23 16:30 PDT, Kyle Huey [:khuey] (
bzbarsky: review+
Details | Diff | Splinter Review

Description Toshihiro Yamada 2012-08-15 20:52:10 PDT
Created attachment 652314 [details]
html and background image for testing

When I open html file in the attached archives, back ground image isn't shown at all.

Regression range(mozilla-central)

So, does a0bddf5fcb91(merge form b-s to m-c) cause the problem?
Comment 1 Ekanan Ketunuti 2012-08-16 07:29:19 PDT
first bad in build-system:

+khuey on this.
Comment 2 Kyle Huey [:khuey] ( 2012-08-16 07:43:56 PDT
I can reproduce this.
Comment 3 Octoploid 2012-08-17 02:25:07 PDT
Probably a dup of
Comment 4 Kyle Huey [:khuey] ( 2012-08-17 06:57:07 PDT
Comment 5 Kyle Huey [:khuey] ( 2012-08-17 16:00:09 PDT
Created attachment 652962 [details] [diff] [review]
Comment 6 Boris Zbarsky [:bz] 2012-08-17 17:02:42 PDT
Can I please have a checkin comment explaining what the patch is doing and why?

Also, is this reftestable?
Comment 7 Kyle Huey [:khuey] ( 2012-08-17 17:38:56 PDT
I was assuming you wouldn't look at this until the work week next week ;-)

The problem here is that the nsCSSValue we get in nsGenericHTMLElement::MapBackgroundInto is a temporary, so the nsCSSValue::Image we put into that value is destroyed once we're done doing style stuff.  This worked fine before Bug 697230, because the nsImageLoader would grab the request off the nsCSSValue::Image and hold it alive.  Bug 697230 changed the behavior here; now when the nsCSSValue::Image is destroyed it tells the image loader to drop the request.  The result is that all the references to the request are dropped and the frame is never told it has a background.

The solution is to keep the nsCSSValue::Image alive longer.  I've done this by adding a new type (well, two, but we'll get there in a sec) of nsAttrValue that holds a strong ref to a nsCSSValue::Image.  The nsAttrValue persists for the lifetime of the attribute which ensures that our image request stays alive.

I ran into two problems.  The first is that nsAttrValue's hash code needs to be immutable, or we'll assert during teardown because we wouldn't be able to remove nsMappedAttributes from the hashtable on nsHTMLStyleSheet.  After some discussion with dbaron I implemented two new nsAttrValue types, one holding a nsCSSValue::Image and one holding a nsCSSValue::URI.  In ParseAttribute we create a nsCSSValue::URI and assign it to the nsCSSValue.  Later, in MapBackgroundInto, we create a nsCSSValue::Image from the nsCSSValue::URI and mutate the nsAttrValue.  This allows us to easily ensure the hash code does not change, and makes this mirror the setup for actual CSS images.

The second problem I ran into is that nsAttrValue.h gets included in external linkage code (test_IHistory.cpp, via Link.h and Element.h).  nsCSSValue.h cannot be included in external linkage code, because it uses the internal string headers.  I also can't forward declare nsCSSValue::[URI|Image] because they are nested classes.  Instead I pulled them out and moved them to a namespace, but left in typedefs so I didn't have to change the whole tree here (I can do that in a followup if you like).  I ran that by dbaron and he was mostly ok.

Finally after some archaeology dbaron and I are convinced that the quirks mode code here was rendered inoperative by bug 237078 and can be removed.

It is reftestable.  There's a test in the patch, in fact.
Comment 8 Kyle Huey [:khuey] ( 2012-08-17 17:40:31 PDT
Also there are two comments in MapBackgroundInto (one about xml:base and one about principals).  Since I've moved part of that code into ParseAttribute, those can both be fixed.  I fixed the principal one here; somebody can do xml:base in another bug if that's something people care about.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-08-17 17:46:36 PDT
(In reply to Kyle Huey [:khuey] ( from comment #7)
> Finally after some archaeology dbaron and I are convinced that the quirks
> mode code here was rendered inoperative by bug 237078 and can be removed.

In particular, because the only things the background="" could plausibly override (given that MapBackgroundAttributesInto is used for body, table, tbody, thead, tfoot, tr, th, and td) are:
 * user styles, where behavior shouldn't be a quirk
 * background:inherit in the UA style sheet, coming in particular from the line removed in bug 237078 (and moved from html.css to quirk.css in bug 4510)
Comment 10 Kyle Huey [:khuey] ( 2012-08-21 13:51:47 PDT
*** Bug 784481 has been marked as a duplicate of this bug. ***
Comment 11 Kyle Huey [:khuey] ( 2012-08-21 21:06:17 PDT
Created attachment 654068 [details] [diff] [review]
Better patch

This one actually compiles on gcc.
Comment 12 Boris Zbarsky [:bz] 2012-08-22 18:40:00 PDT
Comment on attachment 654068 [details] [diff] [review]
Better patch

>+++ b/content/base/src/nsAttrValue.h
>-    ,eAtomArray =      0x11

r=me on removing all this manual numbering if desired!

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+    if (aAttribute == nsGkAtoms::background) {

This is slightly suboptimal because it will do the "parse it into a URI" thing for all HTML elements, but it's only used by body/td/th/table/tr/tbody/thead/tfoot...

It might be better to just have a helper method for doing this that the ParseAttribute methods for those classes call.  On the other hand, maybe people don't put this attribute on other elements much anyway, in which case it's just an extra branch per attr set....

>+      // XXX this breaks if the HTML element has an xml:base
>+      // attribute (the xml:base will not be taken into account)
>+      // as well as elements with _baseHref set. We need to be able
>+      // to get to the element somehow, or store the base URI in the
>+      // attributes.

Bah.  Just nix this comment and GetBaseURI() here.  Just make sure to not leak it.  ;)

>+      // Note that this should generally succeed here, due to the way
>+      // |spec| is created.  Maybe we should just add an nsStringBuffer
>+      // accessor on nsAttrValue?
>+      nsString value(aValue);
>+      nsRefPtr<nsStringBuffer> buffer = nsCSSValue::BufferFromString(value);

The comment there doesn't make sense.  Shouldn't it refer to "value" instead of "spec"?  In any case, having an nsStringBuffer accessor on nsAttrValue would certainly not help the situation anymore.

>+      mozilla::css::URLValue *url =
>+        new mozilla::css::URLValue(buffer, doc->GetDocBaseURI(), uri,

And use the base URI from above.

>@@ -2734,54 +2765,31 @@ nsGenericHTMLElement::MapBackgroundInto(
>+      nsString spec;
>+      value->ToString(spec);
>+      if (!spec.IsEmpty() && value->Type() == nsAttrValue::eURL) {

How about you check the type before you start calling ToString?

And maybe the conversion to a css::ImageValue should live on nsAttrValue?  Then it could just keep its out-of-band string and you wouldn't need the extra ToString and string copy on SetTo.

>+++ b/layout/style/ImageLoader.cpp
>-ImageLoader::MaybeRegisterCSSImage(nsCSSValue::Image* aImage)
>+ImageLoader::MaybeRegisterCSSImage(ImageLoader::Image* aImage)

Pretty sure you don't need the "ImageLoader" bit there on the argument.  Same in various other places in this file.

r=me with the above fixed.
Comment 13 Kyle Huey [:khuey] ( 2012-08-23 16:30:34 PDT
Created attachment 654837 [details] [diff] [review]

Review comments addressed, and some new tests added.
Comment 14 Boris Zbarsky [:bz] 2012-08-23 22:05:54 PDT
Comment on attachment 654837 [details] [diff] [review]

Comment 15 Kyle Huey [:khuey] ( 2012-08-24 10:53:19 PDT
Comment 16 Matthias Versen [:Matti] 2012-08-24 11:09:26 PDT
*** Bug 785352 has been marked as a duplicate of this bug. ***
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:01:31 PDT
Comment 18 Toshihiro Yamada 2012-08-25 06:47:07 PDT
Confirmed fixed with the build ead893fc780c(m-c).

Thank you for your great works!
Comment 19 Kyle Huey [:khuey] ( 2012-08-25 08:44:05 PDT
Thank you for the quick and concise bug report.
Comment 20 Paul Silaghi, QA [:pauly] 2012-10-17 06:52:11 PDT
Saw the issues on Nightly 2012-08-21.
The testcase and looks OK on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0b1
Verified fixed.

Note You need to log in before you can comment on or make changes to this bug.