Closed Bug 783162 Opened 8 years ago Closed 8 years ago

Background image isn't shown at all

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox17 + verified

People

(Reporter: yamadat501, Assigned: khuey)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Attached file html and background image for testing (obsolete) —
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?
Keywords: regression
first bad in build-system: https://tbpl.mozilla.org/?tree=Build-System&rev=54f586a529ae

+khuey on this.
I can reproduce this.
Assignee: nobody → khuey
Status: UNCONFIRMED → ASSIGNED
Component: General → Style System (CSS)
Ever confirmed: true
Product: Firefox → Core
Attached patch Patch (obsolete) — Splinter Review
Attachment #652314 - Attachment is obsolete: true
Attachment #652962 - Flags: review?(bzbarsky)
Can I please have a checkin comment explaining what the patch is doing and why?

Also, is this reftestable?
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.
Blocks: 697230
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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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)
Attached patch Better patch (obsolete) — Splinter Review
This one actually compiles on gcc.
Attachment #652962 - Attachment is obsolete: true
Attachment #652962 - Flags: review?(bzbarsky)
Attachment #654068 - Flags: review?(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.
Attachment #654068 - Flags: review?(bzbarsky) → review+
Attached patch PatchSplinter Review
Review comments addressed, and some new tests added.
Attachment #654068 - Attachment is obsolete: true
Attachment #654837 - Flags: review+
Attachment #654837 - Flags: review+ → review?(bzbarsky)
Comment on attachment 654837 [details] [diff] [review]
Patch

r=me
Attachment #654837 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 785352
https://hg.mozilla.org/mozilla-central/rev/8bd8ec63a020
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Confirmed fixed with the build ead893fc780c(m-c).

Thank you for your great works!
Thank you for the quick and concise bug report.
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 816498
Depends on: 817531
You need to log in before you can comment on or make changes to this bug.