Bug 17897 – Not Rendering Images Imported from XHTML Document

• Component: DOM
• Status: RESOLVED FIXED

[Weston Ruter]

If an XMLHttpRequest is made for an XHTML document (with MIME type application/xml+xhtml) and an element containing images in the responseXML document is inserted into the requesting page, the images are not able to load. If, however, the same page is loaded in the browser (without the XHR request) then the images are displayed successfully.

There is a workaround, however, to this issue. After appending the element, simply re-set each image's src DOM or content attribute.

var imgs = div.getElementsByTagName('img');
for(var i = 0; i < imgs.length; i++){
    imgs[i].src = imgs[i].src;
}

See associated URL for example of issue.

[Alexey Proskuryakov]

Confirmed with r31120.

[Alexey Proskuryakov]

<rdar://problem/5827614>

[Weston Ruter]

The example URL went 404, as well some of the test images. I fixed them.

[Julien Chaffraix]

I have been able to find the root of this issue:

- the images are not loaded as we load them in a document that is not rendered (because it is loaded via XMLHttpRequest).

- when inserting the images into a rendered document, we do not call their loader to fetch the images so nothing appear on the screen.

- modifying the src attribute forces us to fetch the images by calling the loader.

Taking the bug as I have a fix that I will attach here tomorrow, once I have tested it a bit more.

[Julien Chaffraix]

Created attachment 22964 [details]
Bug fix: fetch image when inserted if necessary and add an error flag in the loader

Sorry for the delay. The patch caused a test failure that I could not determine whether it was caused by the patch or was just rampant.

I seems that it is not failing anymore in ToT so it is not the patch's fault (already filed bug20366 for the test failure under some conditions).

[Eric Seidel (no email)]

Comment on attachment 22964 [details]
Bug fix: fetch image when inserted if necessary and add an error flag in the loader

So are new HTMLImageLoader objects created every time a load is issued?  Otherwise I fear your patch will break things like:

image.src = "willfail.png"
// first load returns an error, possibly from the error callback we do:
image.src = "willwork.png"

Also, I'm not a big fan of functions being declared in the middle of function calls:

 26                 icon.addEventListener("load", function () {
 27                     var console = document.getElementById('console');
 28                     // If the image was loaded correctly, then height and width are not
 29                     // zero.
 30                     if (icon.height && icon.width)
 31                         console.innerHTML = "PASSED";
 32                     else
 33                         console.innerHTML = "FAILED";
 34 
 35                     if (window.layoutTestController)
 36                         layoutTestController.notifyDone();
 37 
 38                 }, true);

I'd rather see that declared in a local variable and used instead of as part of that function call.

Marking r- for now.  If you are convinced that the "load works after error case" is OK (i.e. that we already have a test case which checks that and it passes), then you can go ahead and land this.  Otherwise we'll need a test case to prove that the "load a new url after the first one fails" case works.

[Julien Chaffraix]

(In reply to comment #6)
> (From update of attachment 22964 [details] [edit])
> So are new HTMLImageLoader objects created every time a load is issued?

It is created once and reused between loads.
 
> Otherwise I fear your patch will break things like:
> 
> image.src = "willfail.png"
> // first load returns an error, possibly from the error callback we do:
> image.src = "willwork.png"

Actually your test case will work as we return an small image when we try to load a non-existent image.
But you are right, there is an issue if the loader had an error. I will fix that, provide a test case and correct the misleading comment.


> Also, I'm not a big fan of functions being declared in the middle of function
> calls:
> 
>  26                 icon.addEventListener("load", function () {
>  27                     var console = document.getElementById('console');
>  28                     // If the image was loaded correctly, then height and
> width are not
>  29                     // zero.
>  30                     if (icon.height && icon.width)
>  31                         console.innerHTML = "PASSED";
>  32                     else
>  33                         console.innerHTML = "FAILED";
>  34 
>  35                     if (window.layoutTestController)
>  36                         layoutTestController.notifyDone();
>  37 
>  38                 }, true);
> 
> I'd rather see that declared in a local variable and used instead of as part of
> that function call.

Having anonymous function is fine by me, but I will  make the change you are requesting as it is a standalone method that could easily be extracted.

[Julien Chaffraix]

Created attachment 23003 [details]
Updated patch: tackles Eric's comments

[Eric Seidel (no email)]

Comment on attachment 23003 [details]
Updated patch: tackles Eric's comments

I'm still slightly concerned by this patch as it might break existing callers of updateFromElement(), or future callers who don't know about this "don't do anything if I've ever seen an error" behavior.  One way to address that would be to have updateFromElement take a flag which was used to trigger this kind of clear.  Another would be to ASSERT in updateElement that the current source was the same as the previous source, or simply to only not re-try in updateFromElement when the current source was the same and there was already an error.  Another way would be to just give updateFromElement a better name! (the current one is bad because it's so generic)

Hum... I just still think we can make this API clearer.

[Julien Chaffraix]

Created attachment 23215 [details]
Updated patch: Store source url and check it when determining if the loader is in error

> Hum... I just still think we can make this API clearer.

Sure, I have removed the awkward part and stored the source url so that we can determine if it is the same request we are doing (and abort it if it was in error).

[Darin Adler]

Comment on attachment 23215 [details]
Updated patch: Store source url and check it when determining if the loader is in error

> // our loader may have not fetched the image so do it now.

Should have a comma before "so do it now".

> // If we do not have an image here, it means that an error
> // occurred (cross-site violation...).

I don't understand the meaning of "..." in this comment.

  class HTMLImageLoader : public CachedResourceClient {
+
  public:

Why are you adding a blank line here? We normally don't do that. The other blank line you added seems fine.

I'm not sure the behavior here is right. If changing the URL triggers a new load, it's possible that re-setting the URL to the same value also should trigger a new load. We should test this in other browsers to be sure we match their behavior.

I'm going to say r=me because I think it's OK to land this patch as-is. But I'd really like to see us test that "does changing the URL to the same value trigger a load" question, because I think it may be needed to match the behavior of other web browsers.

[Julien Chaffraix]

(In reply to comment #11)
> (From update of attachment 23215 [details] [edit])
> > // our loader may have not fetched the image so do it now.
> 
> Should have a comma before "so do it now".

Sure.
> 
> > // If we do not have an image here, it means that an error
> > // occurred (cross-site violation...).
> 
> I don't understand the meaning of "..." in this comment.

I do not know all the possible errors that could occur here. I guess I could just remove it.

>   class HTMLImageLoader : public CachedResourceClient {
> +
>   public:
> 
> Why are you adding a blank line here? We normally don't do that. The other
> blank line you added seems fine.

My bad, I got confused. It will be changed.

> I'm not sure the behavior here is right. If changing the URL triggers a new
> load, it's possible that re-setting the URL to the same value also should
> trigger a new load. We should test this in other browsers to be sure we match
> their behavior.
> 
> I'm going to say r=me because I think it's OK to land this patch as-is. But I'd
> really like to see us test that "does changing the URL to the same value
> trigger a load" question, because I think it may be needed to match the
> behavior of other web browsers.
> 

The patch has rotten and the HTMLImageLoader has been shared with SVG in the ImageLoader so I will need to update it. I will try your use case and add the required test case while updating it. Thanks!

[Eric Seidel (no email)]

Comment on attachment 23215 [details]
Updated patch: Store source url and check it when determining if the loader is in error

Patch has rotten, clearing review flag.

[Julien Chaffraix]

Created attachment 24957 [details]
New iteration: Tackles Darin's comments and match FF & Opera

I have done a bit of testing as asked by Darin: Opera and FF both try to load the new url even if it is the same as the previous one (I could not test on IE).

The attached patch makes us to match FF and Opera. I have added a test case to check that we try to load the image even if it was in error and we set the 'src' attribute to the existing value.

[Darin Adler]

Comment on attachment 24957 [details]
New iteration: Tackles Darin's comments and match FF & Opera

> -            m_imageLoader->updateFromElement();
> +            m_imageLoader->updateFromElement(true);

The reason I don't like boolean arguments to functions like this is it's so hard to understand them at the call site. It would be a lot nicer if that argument had a name like TryAgain instead of just being true, or if this was a separate function with a sensible name, even if it only ends up being a boolean argument anyway.

> -void ImageLoader::updateFromElement()
> +void ImageLoader::updateFromElement(bool shouldIgnorePreviousLoadFailure)

You could also call this shouldTryLoadingAgain, or shouldRetryLoad. Perhaps a bit less precise but also maybe a little easier to understand.

> +    AtomicString m_sourceURL;
>      bool m_firedLoad : 1;
>      bool m_imageComplete : 1;
>      bool m_loadManually : 1;
> +    bool m_hadError : 1;

After reading the logic carefully, I realize that you only need to store the old URL in the case where the load failed. I think we could change this to work that way. Then you could have m_failedLoadURL instead of m_sourceURL and not bother with m_hadError at all. I think the logic would stay simple and clear in that case.

r=me as-is, but we could consider making a little further improvement.

[Julien Chaffraix]

Created attachment 25079 [details]
Another round: Remove the boolean and 

> The reason I don't like boolean arguments to functions like this is it's so
> hard to understand them at the call site. It would be a lot nicer if that
> argument had a name like TryAgain instead of just being true, or if this was a
> separate function with a sensible name, even if it only ends up being a boolean
> argument anyway.

> After reading the logic carefully, I realize that you only need to store the
> old URL in the case where the load failed. I think we could change this to > work
> that way. Then you could have m_failedLoadURL instead of m_sourceURL and not
> bother with m_hadError at all. I think the logic would stay simple and clear > in
> that case.

I have given it some thought and rewritten the error handling part to remove the m_hadError bool (as you were rightly pointing out). I also used this iteration to remove the boolean and add a method updateFromElementIgnoringPreviousError. This should address the two previous comments.

[Darin Adler]

Comment on attachment 25079 [details]
Another round: Remove the boolean and 

I'm going to say review+ on this. I think the comments are too repetitive, but this is otherwise very good.

[Julien Chaffraix]

Landed the patch in r39104 after having factored out the comments into ImageLoader.h and tweaked the svg test case to avoid adding a platform specific test case.