219302 - [FIXr]Infinite loop creating http channels for astrophy

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Builds:
  2003091405 seamonkey
  20030811   firebird

Loading the URL, I get necko to go through infinite loops and the astrophy image
(http://www.hixie.ch/resources/images/astrophy/original) is never displayed. 
Here is a snippet from my necko log of the 914 build, which is repeated ad
infinitum (only differing in the ptr references)

1024[80ad5b8]: nsHttpHandler::NewURI
1024[80ad5b8]: nsHttpHandler::NewProxiedChannel [proxyInfo=0]
1024[80ad5b8]: Creating nsHttpChannel @85dbaf8
1024[80ad5b8]: nsHttpChannel::Init [this=85dbaf8]
1024[80ad5b8]: host=www.hixie.ch port=-1
1024[80ad5b8]: uri=http://www.hixie.ch/resources/images/astrophy/original
1024[80ad5b8]: Creating nsHttpConnectionInfo @86f7370
1024[80ad5b8]: nsHttpHandler::AddStandardRequestHeaders
1024[80ad5b8]: nsHttpChannel::SetRequestHeader [this=85dbaf8 header="Accept"
value="image/png,image/jpeg,image/gif;q=0.2,*/*;q=0.1" merge=0]
1024[80ad5b8]: nsHttpChannel::AsyncOpen [this=85dbaf8]
1024[80ad5b8]: nsHttpHandler::OnModifyRequest [chan=85dbaf8]
1024[80ad5b8]: nsHttpChannel::Connect [this=85dbaf8]
1024[80ad5b8]: nsHttpChannel::OpenCacheEntry [this=85dbaf8]
1024[80ad5b8]: nsHttpHandler::NewURI
1024[80ad5b8]: nsHttpChannel::Cancel [this=85dbaf8 status=804b0002]
1024[80ad5b8]: nsHttpChannel::OnCacheEntryAvailable [this=868a948 entry=86eca80
access=2 status=0]
1024[80ad5b8]: channel was canceled [this=868a948 status=804b0002]
1024[80ad5b8]: nsHttpChannel::CloseCacheEntry [this=868a948 status=804b0002]
1024[80ad5b8]: dooming cache entry!!
1024[80ad5b8]: nsHttpChannel::AsyncAbort [this=868a948 status=804b0002]
1024[80ad5b8]: nsHttpChannel::Cancel [this=8672938 status=80540005]
1024[80ad5b8]: Destroying nsHttpChannel @8672938
1024[80ad5b8]: Destroying nsHttpConnectionInfo @865eb10

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Stack trace from the first call to nsHttpChannel::Cancel()

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Could this be a regression from the patch to bug 196797?

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Ok, backing that patch out helps somewhat.  Astrophy finally renders, but the
browser is still looping.  I bet that the patch bug 83774 also has some effect here.

Absolving darin, shoving blame to bz.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Oh I forgot to mention that I see this in 1.4alpha, but not 1.3final.  Which
also points to bz.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Astrophy finally renders, but the browser is still looping.  

Then it helps not at all; whether the image renders is not relevant as long as
the looping behavior is present, is it?

What I _suspect_ may be happening here is that the aURI and oldURI are not equal
in nsImageLoader::Load because the channel gets redirected.  Is that the case? 
If so, perhaps we need to save aURI in that method and use that for oldURI
instead of getting the uri off of mRequest...

Any chance you could either post what aURI and oldURI are there or even try the
proposed change, Chris?

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

The root of the problem seems to stem from this rule pair in the testcase:

   BODY { border: solid lime; background: green url(/resources/images/box-12x12)
center top no-repeat; color: white; }
   HTML { border: solid blue; background: white
url(/resources/images/astrophy/original) repeat-x top center; color: black; }


GetFrameForBackgroundUpdate() in nsCSSRendering is returning the same frame to
for both the BODY and HTML elements.  So when the same frame gets passed into
the pres context to load the image, it keeps grabbing the same loader, and the
URI is of course different each time (as the stack trace I attached shows since
it can only get to that line to make the Cancel() call if the uris are different).

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

I'm actually not sure how this worked in the past... This function hasn't
changed in ages.  Did the image loader just not care if passed in different URIs?

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yes, I see from the patch to bug 196797 that it did not in the past.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I'm actually not sure how this worked in the past...

As you point out, it didn't, really... Before bug 196797 we would simply start a
new load and forget about the old one, then keep kicking off new loads on every
paint, which would lead to the looping you see.

Frankly, I'm not quite sure why we have this code at all.  The canvas
background-painting seems to also be handled by the call to FindBackground() at
the beginning of PaintBackground() (if the frame is the canvas frame, this looks
up the right style context and kicks off the background load with the canvas
frame as the target frame).  Therefore, it seems to me that the
GetFrameForBackgroundUpdate thing is just wrong (since the background loads that
require invalidating the canvas should actually have the canvas frame as the
frame that the image loader has).

Or am I missing something?

Comment 10

[Boris Zbarsky [:bzbarsky]]

The core issue here is that the canvas paints the background it gets off the
body..... but the body also paints its background, except it invalidates the
canvas to do it.  Same for <html>.  If the canvas grabbed the background from
one of those nodes, the node itself should not be painting it.  If it's
painting, it should just paint on itself (so that in this case, the canvas would
get the <html> background, and the <html> would paint nothing, but the <body>
would paint its background within its own border box).

Comment 11

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

If we start loading the images from the style code, this should go away, I assume?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, that should help.   In fact, almost any fix for bug 57607 will help.

Comment 13

[Thamer Mahmoud]

is this a dupe of Bug 110113 ?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Maybe, except this one has less noise and a clearer description of the
underlying problem.

Comment 15

[Wayne Woods]

So, this bug is dependent on bug 57607 which is, in-turn, dependent on bug 113173

A fix seems to have been checked in for the latter bug, but only on the trunk.
Does that mean the target milestone should be shifted for this bug? I suppose
it's too late to expect all three bugs to be fixed on the branch at this stage?

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Upon some scouring of this code, GetFrameForBackgroundUpdate is 1) bogus and 2)
not needed.

As things stand, the frame that propagated its background to the canvas never
paints its background.	Therefore, the only time we would want the canvas frame
as the target of the image load there is if the canvas frame is painting.  Then
it's aForFrame.  The body should _not_ be targeting the canvas if it's loading
its own background.  Dynamic changes are handled by the code in
ApplyRenderingChangeToTree over in nsCSSFrameConstructor.

I have tested this page in the following ways:

1)  Load a page in which the background of <body> is propagated to the canvas.
    Change the <body> background via document.body.style and see whether we
    repaint the canvas.
2)  Load a page a la hixie's test, but in which the canvas background has a
    5-second server-side delay.  Make sure that after 5 seconds we paint the
    background on the whole canvas as needed.

Both work fine.  This bug is gone.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fix checked in for 1.7b.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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