57607 - (latebg) background images do not load in background windows (not loaded until paint)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Eugene Savitsky]

1. Open at least 2 mozilla windows (1 and 2)
2. Enter any URL with big BG pic (for expl: the given URL) to URL bar at one of 
them (1)
3. hit enter and quick go to the other window (2)
4. In 2 open some link
5. Now go back to 1 and if you are lucky you should see the page, but the 
background is reflowing (loaded, reloaded?). 

It happens not every time, but I have seen this many-many time. I'm not sure 
about the way I suggested to repro it, but it defenetly happens only when 
switching from one window to another.

I saw this on the URL once I visited it for a few days ago.

2000102108

Comment 1

[Asa Dotzler [:asa]]

over to layout

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

XML/DOM team's turn to triage Clayton's list.

Comment 3

[harishd]

Triaging clayton's bug list:
------------------------------

To me it looks like some kind of a JS problem. Giving bug to JS people to
investigate.

Comment 4

[harishd]

Oops wrong bug!!! back to me.

Comment 5

[harishd]

Not sure to whom this bug should go. Starting with Pam.

Comment 6

[Eugene Savitsky]

I believe the same thing happens on www.fcenter.ru. Any sugestion?

Comment 7

[ivo]

If I am not terribly mistaken, bug 65459 has similar nature.

Comment 8

[pnunn]

This sounds more like a paint problem rather than a 
decoding problem to me. Reassigning to the rendering folk.

Comment 9

[Martin Horwath]

*** Bug 70669 has been marked as a duplicate of this bug. ***

Comment 10

[Kevin McCluskey (gone)]

I'm not seeing the problem in a current build on WINNT build id 2001030904.
Marking WORKSFORME.

Comment 11

[Aaron Kaluszka]

I still see the problem on Build 20010522/WinXP.
Suggest rewording summary: Background image does not load when window is hidden
Also, step 4 on reproduction steps doesn't need to take place.

Comment 12

[Marc Attinasi]

*** Bug 65459 has been marked as a duplicate of this bug. ***

Comment 13

[Aaron Kaluszka]

This no longer occurs

Comment 14

[ivo]

Mozilla 2001112003

This bug is not fixed. This bug is there. If You do not see it, this will not 
make it go away. See how to see this bug at bug 65459. 

I typed www.athlonmb.com, pressed [enter], minimized this window, took a long 
pause, deminimized this window again and magically background pictures just 
started loading. 

Comment 15

[Aaron Kaluszka]

You are correct, I marked the wrong bug, sorry.

Comment 16

[Kevin McCluskey (gone)]

Build moving all existing future-P3 bugs to future-P4.

Comment 17

[Kurt Pruenner]

Here's an even easier method to reproduce this:

Activate tabbed browsing, check the "Load links in the background" pref and just
middle click on a link that goes to a page which exposes this problem to open it
in a new tab in the background.

Paste the following data:-URL into the location bar as a test case:

data:text/html,<a href="http://www.battle.net/diablo2exp/">Link</a>

All table backgrounds on that page only get loaded after switching to the new
tab; and I've even encountered some pages where not having the backgrounds
loaded yet had funny effects like white text on white background... that sure
does hamper your good browsing experience... :(

Comment 18

[Boris Zbarsky [:bzbarsky]]

*** Bug 121787 has been marked as a duplicate of this bug. ***

Comment 19

[Boris Zbarsky [:bzbarsky]]

*** Bug 143274 has been marked as a duplicate of this bug. ***

Comment 20

[Judson Valeski]

minusing for now. if a patch emerges, please re-nominate.

Comment 21

[Kevin McCluskey (gone)]

This currently works this way by design. The background image load is initiated
from a paint. 

Comment 22

[Kevin McCluskey (gone)]

nsbeta1+

Comment 23

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 24

[junk]

>>This currently works this way by design. The background image load is initiated
from a paint. 

This is bad design, don't you think? Causes annoying delays. Same issue with
images that are initially display:none. Causes probs with menus and other
dynamic segments.

Comment 25

[Hixie (not reading bugmail)]

This is an even bigger problem now we have tab groups. It looks silly to have
the background pop in as you change tabs.

Comment 26

[Jaime Rodriguez, Jr.]

Topembed- as this is not currently blocking a major embedding customer.

Comment 27

[praveen tummala]

Attachment: observe the background image being repainted

Observed it on w2k 2002050708

Comment 28

[Chris Nalls]

Confirming Topembed-

Comment 29

[Kevin McCluskey (gone)]

nsbeta1-. Not going to be able to get to this for the next release.

Comment 30

[http://tinymailto.com/oliversl]

It also happends with <TD> elements, like this:
<td background="">

Confirmed in Mozilla 1.3b (BuildID 2003021008)

Comment 31

[Boris Zbarsky [:bzbarsky]]

*** Bug 155962 has been marked as a duplicate of this bug. ***

Comment 32

[Boris Zbarsky [:bzbarsky]]

*** Bug 93301 has been marked as a duplicate of this bug. ***

Comment 33

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 208384 has been marked as a duplicate of this bug. ***

Comment 34

[Boris Zbarsky [:bzbarsky]]

*** Bug 209132 has been marked as a duplicate of this bug. ***

Comment 35

[Chris Casciano]

-> All/All (this occurs on OS X as well)

another obvious case of this can be seen with the bottom attached background
image on this page (and just about any use of css based background images i've
ever done): http://placenamehere.com/neuralustmirror/200202/fun.php?sheet=15

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 217976 has been marked as a duplicate of this bug. ***

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the way to fix this is to fix bug 113173, and then convert the image
type in that bug to some sort of image structure that causes the image load to
start.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Indeed; hence the dependency.

Comment 39

[Boris Zbarsky [:bzbarsky]]

*** Bug 223690 has been marked as a duplicate of this bug. ***

Comment 40

[Oleg Sidletskiy]

*** Bug 216085 has been marked as a duplicate of this bug. ***

Comment 41

[Adam Becevello]

*** Bug 228908 has been marked as a duplicate of this bug. ***

Comment 42

[Wayne Woods]

Just wondering if this bug is fixed now. I couldn't reproduce it when I tried
today, and the bug that it depends on (bug 113173) is marked as fixed.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Yes, this bug is still alive and well.  All you need is a slow link and a
cleared cache to easily reproduce it.  ;)

Comment 44

[Wayne Woods]

Ah, so I now see, sorry :) Far more obvious on a 56k than ethernet ;)

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 231703 has been marked as a duplicate of this bug. ***

Comment 46

[Boris Zbarsky [:bzbarsky]]

So the goals here are twofold:

1)  Parsing a background URL should _not_ immediately load it.  See
    http://weblogs.mozillazine.org/hyatt/archives/2004_02.html#004874 (the CSS
    load improvements section).
2)  The image load should start as early as possible once we know we really need
    it.

One possible proposal is as follows:

A)  Store the background as a nsIURI* at first (in the nsCSSDeclaration)
B)  The first time we compute an nsStyleBackground that wants that background,
    kick off the image load.  This can be done by
    nsCSSCompressedDataBlock::MapRuleInfoInto, perhaps; see the check we already
    make for eCSSProperty_font_family.  The compressed data block will naturally
    save the imgIRequest* pointer in its nsCSSValue (which will need a new type
    to handle that and all).  This has the problem that to do the load you need
    the document (for security checks, if nothing else).  We don't have that in
    the data block.  Maybe the block could just update itself and the
    nsCSSStyleRule could have logic to do the load?

This is all great, but the problem here, of course, is that as the image comes
in the relevant frames will need to be invalidated.  At the moment, I see no way
to accomplish this given the suggestion above....  There are several options I see:

1)  Give the nsCSSCompressedDataBlock a way to invalidate frames or something
    (bad idea).
2)  Make it possible to add an observer to an imgIRequest (all the frames that
    get that struct could add themselves).  Pav is likely to protest, I
    guess
3)  Have the load start in nsCSSCompressedDataBlock and keep the nsImageLoader
    stuff as-is; rely on libpr0n to coalesce the loads.  This will do a bunch of
    work, check cache, sometimes rehit the network, etc, whereas we just want to
    use that nice imgIRequest we have there.  When it works, it's equivalent to
    #2.
4)  Do the load from some other totally different spot (eg bug 162253).  That
    still has all the same invalidation problems.

The invalidation problem is an issue no matter what we do here.  Really, option
#2 is the simplest one, in my mind.  Everything else is just working around
failings in the libpr0n api....

Note that if option #2 were taken we could remove a bunch of code from
nsImageLoadingContent too (code that handles delivering the notifications it
gets to all the imgIDecoderObservers registered on it).

Comment 47

[Andrew Hagen]

Owner is "gone."

Comment 48

[Boris Zbarsky [:bzbarsky]]

I talked to pav, and he's ok with having an imgILoader method that takes an
imgIRequest and an observer and returns a new imgIRequest for that observer.  So
we could even keep using the prescontext's nsImageLoader stuff as we fix this
bug (though it could use some cleanup, really...).

There's still the question of how exactly to munge the style data to work right
(in such a way that background image attributes will work too)....

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 235015 has been marked as a duplicate of this bug. ***

Comment 50

[Boris Zbarsky [:bzbarsky]]

> There's still the question of how exactly to munge the style data to work right

Well, the compressed data block can just kick off image loads -- nsRuleData has
a prescontext member it could use to get the document and such.

Background image attributes are probably best handled by just kicking off the
image load in the attr mapping code as needed (and just having the style struct
be the only thing that's holding on to the imgIRequest).

Comment 51

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This just does background images.  I'd like to do list-style-image too, but XUL
does some weird stuff with that one, so I'd like to put that off into a
separate patch.  Similarly for url() generated content, maybe (need to think
about that some more).

Comment 52

[Boris Zbarsky [:bzbarsky]]

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

Stuart, could you take a look at the imgIRequest.idl and imgRequestProxy.cpp
changes?  Putting this functionality there seemed more natural than sticking it
on imgILoader, but I suppose I could move it over to imgILoader instead, I
guess....

David, the rest of this needs your review, really.  The basic change is the
introduction of the nsCSSValue::Image type which holds the imgIRequest in
addition to the nsCSSValue::URL (the latter stores the original string and
handles equality comparisons).	The rest is fallout from this and a bit of
cleanup of image loading in general to share some code.  Ideally, I would like
to move towards doing as much image loading as possible through the
nsContentUtils methods so that we don't have to do complicated security checks
all over...

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

The Image nested class needs an mString member for serialization, etc., just
like URL does.	Given that, I think it should just inherit from URL.

I also don't see why GetImageValue would require you to include imgIRequest.h
-- just forward declaring (as you do) should be sufficient.

Comment 54

[Boris Zbarsky [:bzbarsky]]

> I also don't see why GetImageValue would require you to include imgIRequest.h

Because returning an nsIFoo* out of an nsCOMPtr<nsIFoo> involves instantiating
and nsDerivedSafe<nsIFoo>, which you can't do if nsIFoo is only forward-declared.

I'll work on making Image inherit from URL.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

I'm not sure how the whole thing compiles, then, since I thought declaring
nsCOMPtr<nsIFoo> required the header.  Or did we remove the sizeof hack?

Comment 56

[Boris Zbarsky [:bzbarsky]]

We removed the sizeof hack.  See bug 107291.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Image inherit from URL

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143224 [details] [diff] [review]
Make Image inherit from URL

I guess this is 1.8a material at this point..

Pavlov, any chance of an ETA on this review?

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 143224 [details] [diff] [review]
Make Image inherit from URL

>+    // Virtual destructor so we can subclass this
>+    virtual ~URL()

Why?  You shouldn't need one as long as you don't delete via a base-class
pointer, which you shouldn't.

>Index: content/html/style/src/nsCSSValue.cpp
>+nsCSSValue::Image::Image(nsIURI* aURI, const PRUnichar* aString,
>+                         nsIDocument* aDocument)
...
>+  if (mURI &&
>+      NS_SUCCEEDED(nsContentUtils::CanLoadImage(mURI, nsnull, aDocument))) {
>+    nsContentUtils::LoadImage(mURI, aDocument, nsnull,
>+                              loadFlag,
>+                              getter_AddRefs(mRequest));

The use of aDocument here is a bit ugly, no?  Isn't this going to do weird
things if the same image load, from the same CSS value, is triggered from two
documents -- wouldn't it then only block onload for one of them?  Or is there
code elsewhere that makes it block onload for the second?

>Index: content/html/style/src/nsCSSDeclaration.cpp

up to here, although I still have to go through the nsCSSValue changes and make
sure you added everything that you need to add.

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 143224 [details] [diff] [review]
Make Image inherit from URL

>Index: content/html/style/src/nsCSSValue.h
>   void  SetNormalValue();
>-
>+  
> #ifdef DEBUG

Don't add unneeded whitespace.

>Index: content/html/style/src/nsCSSDataBlock.cpp
eCSSUnit_Null, "oops");
>+                        if (iProp == eCSSProperty_background_image &&
>+                            val->GetUnit() == eCSSUnit_URL) {
>+                          nsCSSValue::Image* image =
>+			    new nsCSSValue::Image(val->GetURLValue(),
>+						  val->GetOriginalURLValue(),
>+						  aRuleData->mPresContext->GetDocument());
>+			  if (image) {
>+			    if (image->mString) {
>+			      // Need to convert this into an image.  Get a
>+			      // writable ref.
>+			      nsCSSValue* writableVal =
>+				ValueAtCursor(NS_CONST_CAST(char*, cursor));
>+			      writableVal->SetImageValue(image);
>+			    } else {
>+			      delete image;
>+			    }
>+			  }
>+                        }

Perhaps everything inside this |if| could be moved into an
|nsCSSValue::StartImageLoad(nsIDocument*) const| that does the casting away of
const?

(And do you need an analogous |ConnectToImageLoad(nsIDocument*)const| for loads
that already started?)

Index: content/shared/src/nsStyleStruct.cpp
>+static PRBool EqualImages(imgIRequest *aImage1, imgIRequest* aImage2)
>+{
>+  if (aImage1 == aImage2) {
>+    return PR_TRUE;
>+  }
>+
>+  if (!aImage2 || !aImage2) {

You want to check aImage1.

>+    return PR_FALSE;
>+  }
>+
>+  nsCOMPtr<nsIURI> uri1, uri2;
>+  aImage1->GetURI(getter_AddRefs(uri1));
>+  aImage1->GetURI(getter_AddRefs(uri2));

You want the URI from aImage2.

>+  return EqualURIs(uri1, uri2);

>Index: content/base/src/nsImageLoadingContent.h

Up to here

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Some additional thoughts that should perhaps be a later patch:
 1. we should get the referrer right -- that may require another member of
nsCSSValue::URL
 2. We should use this image loading code for list-style-image as well.
 3. As I mentioned in comment 59, there are issues with blocking onload, I think,
    and anything else we use |aDocument| for.

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 143224 [details] [diff] [review]
Make Image inherit from URL

sr=dbaron if you address the comments other than those in comment 61, although
it would be nice to address those at some point, and I didn't look too closely
at the image code (I'm assuming the reviewer will do that).

Comment 63

[Boris Zbarsky [:bzbarsky]]

> as long as you don't delete via a base-class pointer

Which I'm doing, since the base-class is what does the refcounting.  I suppose I
could override AddRef and Release in the subclass, though.

> Isn't this going to do weird things if the same image load, from the same CSS
> value, is triggered from two documents

To get in that situation, we need to have an nsIStyleRule that's applying to two
documents at once.  This only happens for rules in user and UA sheets at the
moment.  And blocking onload for backgrounds, etc is just something we plan to
use for regression tests; there user and UA sheets are not relevant, no?  So I'm
not sure this is actually a problem...

If we do feel this is a concern, then we simply can't do any of this from style
data, since the different document may have different image-loading permissions
and we would thus need a separate imgIRequest stored for every document using
the nsCSSValue.

> Don't add unneeded whitespace.

Fixed.

> Perhaps everything inside this |if| could be moved

Done.

> And do you need an analogous |ConnectToImageLoad(nsIDocument*)const|

This is just for the "CSSValue is used by multiple documents" case discussed
above, right?

> You want to check aImage1.
> You want the URI from aImage2.

Argh.  Tab-completion...  Thank you for catching those.

I'll upload a patch with those changes once I compile it and test it in case you
want to have a look.

As for comment 61, I assume you want the referrer to be the sheet, not the
document?  I suppose that makes a certain amount of sense... list-style-image I
mentioned in comment 51; I'll file a followup on that once this patch is done.

Comment 64

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, in theory stylesheets can be shared between documents, but right now it
only happens for XUL.  (And given character encoding issues, it should probably
stay that way.)  So I guess never mind about (3).

Comment 65

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to review comments

I added nsCSSValue::StartImageLoad(), added AddRef/Release overrides on Image,
made nsCSSValue::URL.mRefCnt protected instead of private (so Image can access
it), and fixed the image1/image2 mistakes.

Comment 66

[Asa Dotzler [:asa]]

I don't think we'd block the release for this but if this patch makes the
reviews in time for beta, it'd be nice to get. If it doesn't make it for the
freeze, please request approval for any fully reviewed patch. 

Comment 67

[Boris Zbarsky [:bzbarsky]]

Checked in.  This seems to have set back Tp a little bit.... I suspect that's
due to the fact that we now start image loads earlier; we may be running out of
HTTP connections and waiting for image loads to complete before being able to
load scripts and restart the parser.

At least that's my best guess as to what's going on.  If there is a way to test
this theory by upping the http connection limit on btek for a few cycles, that
would be very helpful...

Comment 68

[Boris Zbarsky [:bzbarsky]]

Bug 236889 filed on the list-style-image issue.

Comment 69

[Jesse Ruderman]

The reason for the Tp regression could just that before this bug was fixed,
Mozilla could fire onload before background images loaded.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #69)
> The reason for the Tp regression could just that before this bug was fixed,
> Mozilla could fire onload before background images loaded.

We can still fire Tp before CSS background images have loaded.

My guess is that bz is right and we're just loading images earlier which is
causing more competition for resources.

The link prefetch code has a mechanism for forcing their loads to not compete
with "normal" loads. Maybe we could use that for CSS background images too?

Comment 71

[Boris Zbarsky [:bzbarsky]]

Yeah, I have some thoughts on doing something like that.. I'll probably be
filing a followup bug on that.

Comment 72

[Benjamin Streeck]

So now the background images are loaded before onload is fired... so what is
wrong with that? In my opinion you've managed to fix several bugs at once, the
late loading of background images and the fireing of onload before everything
was loaded. :-)

So leave it the way it is, would be my instinctive answer... no?

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #72)
> So now the background images are loaded before onload is fired...

No. onload can still fire before all CSS background images are loaded.

Comment 74

[Michael Landis]

> No. onload can still fire before all CSS background images are loaded.

Agreed -- the load event fires after the document is loaded, not after all of
the subsidiary elements are loaded, too. Otherwise, a document with 200K of
images wouldn't fire onload for some time, which wouldn't be a good thing for
script components like style switchers, DOM modifiers, etc....

Comment 75

[Boris Zbarsky [:bzbarsky]]

Guys, could we drop that discussion please?  Especially since there seem to be
very few people who have any idea of when we actually fire onload (roc is one of
them, as it happens).

Comment 76

[Nils Winter]

I just tried it out at http://www.csszengarden.com this bug is actually not fixed.


Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.8a1) Gecko/20040520

Comment 77

[Boris Zbarsky [:bzbarsky]]

Nils, steps to reproduce?

Comment 78

[Nils Winter]

I apologize, this bug is really fixed. 

Go to csszengarden.com open all five Designs in Tabs.
Wait until they loadig-text on the tabs vanishes an then open the last Tab. You
will see, that Mozilla is still loading the Background images although it said
it was ready. Therefore I thought this bug was not fixed. Sorry that I judged to
rashly.
Wait until your Modem/Networkcard doesn't show any significant traffic anymore.
And open the other Tabs. You will see, that Mozilla has loaded all the images.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  See comment 73.  That's quite intentional.