Background image isn't shown at all

VERIFIED FIXED in Firefox 17

Status

()

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

People

(Reporter: Toshihiro Yamada, Assigned: khuey)

Tracking

({regression})

Trunk
mozilla17
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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)
OK:3b46b03dff5c
NG:d67c02074ced

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

Updated

5 years ago
Keywords: regression

Comment 1

5 years ago
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
tracking-firefox17: --- → ?

Comment 3

5 years ago
Probably a dup of https://bugzilla.mozilla.org/show_bug.cgi?id=783126
Nope.
Created attachment 652962 [details] [diff] [review]
Patch
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)
tracking-firefox17: ? → +
Duplicate of this bug: 784481
Created attachment 654068 [details] [diff] [review]
Better patch

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+
Created attachment 654837 [details] [diff] [review]
Patch

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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd8ec63a020
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Duplicate of this bug: 785352
https://hg.mozilla.org/mozilla-central/rev/8bd8ec63a020
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

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

Thank you for your great works!
Thank you for the quick and concise bug report.

Updated

5 years ago
status-firefox17: --- → fixed
Depends on: 786854
Keywords: verifyme
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
status-firefox17: fixed → verified
Keywords: verifyme

Updated

5 years ago
Depends on: 816498
Depends on: 817531
You need to log in before you can comment on or make changes to this bug.