783162 - Background image isn't shown at all

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Toshihiro Yamada]

Attachment: 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)
OK:3b46b03dff5c
NG:d67c02074ced

So, does a0bddf5fcb91(merge form b-s to m-c) cause the problem?

Comment 1

[Ekanan Ketunuti]

first bad in build-system: https://tbpl.mozilla.org/?tree=Build-System&rev=54f586a529ae

+khuey on this.

Comment 2

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

I can reproduce this.

Comment 3

[Octoploid]

Probably a dup of https://bugzilla.mozilla.org/show_bug.cgi?id=783126

Comment 4

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

Nope.

Comment 5

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

Attachment: Patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Can I please have a checkin comment explaining what the patch is doing and why?

Also, is this reftestable?

Comment 7

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

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 (Exited; not receiving bugmail, old account, do not use)]

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-4, no longer working on Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 11

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

Attachment: Better patch

This one actually compiles on gcc.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Review comments addressed, and some new tests added.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 654837 [details] [diff] [review]
Patch

r=me

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd8ec63a020

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bd8ec63a020

Comment 18

[Toshihiro Yamada]

Confirmed fixed with the build ead893fc780c(m-c).

Thank you for your great works!

Comment 19

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

Thank you for the quick and concise bug report.

Comment 20

[Paul Silaghi, QA [:pauly]]

Saw the issues on Nightly 2012-08-21.
The testcase and http://www.justpip.co.uk/ looks OK on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0b1
Verified fixed.