Bug 71661 – Image onload callback fires when image loading disabled.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Page Loading

[Ben Murdoch]

Image onload callbacks are fired even when image loading is disabled with WebCore::Settings::setLoadImagesAutomatically(false).

See http://www.ksasq.com/imgload.html. With images disabled (with Safari I'm using the 'Disable Images' option under the 'Develop' menu), click the show button. Notice that the 'loaded' alert is displayed, and the div gets filled out with the loaded images offsetHeight (of 0). If you then enable image loads, the image will be loaded and displayed automatically, but no onload callback will be fired. Testing in other browsers (FF, IE) shows that when image loading is disabled, no onload callbacks are made (i.e. on the test page, no alert is displayed and the div content remains '???').

Note that this does not reproduce in Chrome which as far as I understand uses it's own mechanism for blocking image loading. It's behavior is as expected - no callbacks fired until you click the button with image loading enabled.

[Ben Murdoch]

Having trouble getting this to reproduce under the test runner for a layout test. In the meantime, the simple code change I have to fix it is attached, any feedback on the approach would be appreciated. (This patch fixes the manual test case and seems to make webkit behave more like other browsers. It's just the automated test I'm having difficulty with).

[Ben Murdoch]

Created attachment 113864 [details]
Code

[Ben Murdoch]

Ah, the layout test controller method to disable image loading ends up being asynchronous inside Settings::setLoadsImagesAutomatically. That explains why I wasn't able to get it to reproduce under the test harness. I'll send a complete patch with test, changelog, etc.

[Ben Murdoch]

Created attachment 114034 [details]
Patch and layout test

[Steve Block]

Comment on attachment 114034 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=114034&action=review

> Source/WebCore/ChangeLog:12
> +        Fix this by failing the cached image load if the resource is not in the cache. This behaviour

Does this fix both of the above problems? Maybe make this more clear.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:398
> +       return 0;

If image loading is disabled and there is a cache entry for this image, but the revalidation logic below determines that we need to reload it, will the image load fail? Is this desired behaviour, or should disabling image loads mean that we should accept any cached images without revalidation? Will the bug(s) you're trying to fix occur in this case?

> LayoutTests/fast/images/blocked-images-onload.html:3
> +    <title>Test that onload is fired after image loads unblocked (bug 71661)</title>

It looks like you're not testing this, but the reverse? In the WebCore ChangeLog, you suggest that your fix addresses both cases. If so, should we test for both?

> LayoutTests/fast/images/blocked-images-onload.html:6
> +        if (window.layoutTestController) {

I think it's usual to write something to the page if a test which depends upon the LTC for correct operation is run without it being present.

> LayoutTests/fast/images/blocked-images-onload.html:29
> +            // that the cal to layoutTestController.disableImageLoading() above has taken effect.

s/cal/call

[Ben Murdoch]

Thanks for taking a look Steve! Updated patch is on the way.

(In reply to comment #5)
> (From update of attachment 114034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114034&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Fix this by failing the cached image load if the resource is not in the cache. This behaviour
> 
> Does this fix both of the above problems? Maybe make this more clear.

The patch prevents the onload callback from firing in the case that the image is not cached and image loading is disabled. I will clarify the changelog.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:398
> > +       return 0;
> 
> If image loading is disabled and there is a cache entry for this image, but the revalidation logic below determines that we need to reload it, will the image load fail? Is this desired behaviour, or should disabling image loads mean that we should accept any cached images without revalidation? Will the bug(s) you're trying to fix occur in this case?
> 

Hopefully I didn't change this behavior. I only intend to prevent uncached images from triggering onload callbacks when image loads are disabled.

> > LayoutTests/fast/images/blocked-images-onload.html:3
> > +    <title>Test that onload is fired after image loads unblocked (bug 71661)</title>
> 
> It looks like you're not testing this, but the reverse? In the WebCore ChangeLog, you suggest that your fix addresses both cases. If so, should we test for both?

I will clarify the text in the test. I only intend to test that an onload callback is not fired when image loading is disabled. It doesn't seem possible to unblock image loads with the current DRT/layoutTestController infrastructure.

> > LayoutTests/fast/images/blocked-images-onload.html:6
> > +        if (window.layoutTestController) {
> 
> I think it's usual to write something to the page if a test which depends upon the LTC for correct operation is run without it being present.
> 

Will do.

> > LayoutTests/fast/images/blocked-images-onload.html:29
> > +            // that the cal to layoutTestController.disableImageLoading() above has taken effect.
> 
> s/cal/call

Fixed.

[Ben Murdoch]

Created attachment 114687 [details]
Patch

[Steve Block]

Comment on attachment 114687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114687&action=review

This looks good to me, but I'm not too familiar with this code. Alexey, can you take a look?

> Source/WebCore/ChangeLog:10
> +        if image loads were subssequently re-enabled, we would not get an onload callback at that

s/subssequently/subsequently

> Source/WebCore/ChangeLog:15
> +        enabled or disabled. We should not fire the callback when loading is disabled. (Note that we

'... is disabled and no cached image is found.'

> LayoutTests/fast/images/blocked-images-onload.html:3
> +    <title>Test that no onload callback is fired when image loads are disabled. (bug 71661)</title>

' ... are disabled and no cached image is found.'

[Alexey Proskuryakov]

One non-obvious thing works now is that images all load when you change the preference back to on in Safari. This patch looks like it would break this (CachedResourceLoader::setAutoLoadImages iterates CachedImage objects still in need of loading).

I don't know if this is an intentional behavior, or just an artifact of initial implementation. But we shouldn't change it unless we can agree that the new behavior is at least no worse.

[Alexey Proskuryakov]

Comment on attachment 114687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114687&action=review

> Source/WebCore/ChangeLog:22
> +        Test: fast/images/blocked-images-onload.html

The test could use a better name - nothing blocks the image. Something like load-event-with-disabled-autoload.html would be better.

[Ben Murdoch]

Thanks for looking at this Alexey. you were right there was a regression with re-enabling the automatic loads. I have a new patch that should address this and the other comments you and Steve had.

[Ben Murdoch]

Created attachment 116232 [details]
Patch.

[WebKit Review Bot]

Attachment 116232 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/loader/cache/CachedImage.cpp:123:  One line control clauses should not use braces.  [whitespace/braces] [4]
LayoutTests/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Murdoch]

Created attachment 116233 [details]
Patch -style fixed.

[Ben Murdoch]

Just noticed a problem with the patch thanks to the cr-linux bot. will send a new one :)

[Ben Murdoch]

Created attachment 116241 [details]
Fix crash in last patch :)

Needed to null check the return of CachedResourceLoader::requestResource.

[Steve Block]

Comment on attachment 116241 [details]
Fix crash in last patch :)

View in context: https://bugs.webkit.org/attachment.cgi?id=116241&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:487
> +    bool shouldLoadNow = true;

is this easier to follow ...

bool shouldNotLoadNow = type == CachedResource::ImageResource && !autoLoadImages() && !inCache;

> LayoutTests/fast/images/load-event-with-disabled-autoload.html:3
> +    <title>Test that no onload callback is fired when image loads are disabled and no cached image is found. (bug 71661)</title>

I guess it's too hard to modify LTC to allow image loading to be re-enabled so we can test that we then get an onload callback?

> LayoutTests/fast/images/load-event-with-disabled-autoload.html:31
> +            window.setTimeout(function() { document.getElementById('img').src = "resources/blocked-images-onload.jpg"; }, 0);

Wrong file name. I'm surprised the test still passes?! Also, is there a generic image we can re-use, rather than adding a new one?

> LayoutTests/fast/images/load-event-with-disabled-autoload.html:34
> +            window.setTimeout(function() { passed(); }, 1000);

Can we be sure that the image isn't cached? Similarly, can we test the case where the image is cached?

[Ben Murdoch]

(In reply to comment #17)
> (From update of attachment 116241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116241&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:487
> > +    bool shouldLoadNow = true;
> 
> is this easier to follow ...
> 
> bool shouldNotLoadNow = type == CachedResource::ImageResource && !autoLoadImages() && !inCache;
> 

It means you get a double negative in the if (!shouldNotLoadNow) a few lines later, but I guess it's fine.

> > LayoutTests/fast/images/load-event-with-disabled-autoload.html:3
> > +    <title>Test that no onload callback is fired when image loads are disabled and no cached image is found. (bug 71661)</title>
> 
> I guess it's too hard to modify LTC to allow image loading to be re-enabled so we can test that we then get an onload callback?
> 

I guess it's technically not that hard, but it's a lot of work to update all the different versions of DRT and ensure they work correctly, I'm not sure it's worth the effort...

> > LayoutTests/fast/images/load-event-with-disabled-autoload.html:31
> > +            window.setTimeout(function() { document.getElementById('img').src = "resources/blocked-images-onload.jpg"; }, 0);
> 
> Wrong file name. I'm surprised the test still passes?! Also, is there a generic image we can re-use, rather than adding a new one?

Good spot - I guess the test still passes because in that case we won't get an onload callback because nothing was loaded... but for the wrong reason. Fixed. The images seemed to named after the tests that they are used in, and I wanted a new filename to ensure it couldn't be stored in a cache anywhere.
> 
> > LayoutTests/fast/images/load-event-with-disabled-autoload.html:34
> > +            window.setTimeout(function() { passed(); }, 1000);
> 
> Can we be sure that the image isn't cached? Similarly, can we test the case where the image is cached?

I added a random query string to the filename, I think that should ensure it's not cached. I added a test in the case that the image is cached we get the callback, but WebKit's behavior in this case is correct currently so it's more a documentation of expected behavior rather than a regression that this patch fixes (i.e. the new test passes with or without my patch).

Thanks for the review, will upload a new patch!

[Ben Murdoch]

Created attachment 116521 [details]
Patch.

[Alexey Proskuryakov]

Comment on attachment 116521 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=116521&action=review

Behavior described in ChangeLog sounds just right. It would be easier for me to review if m_autoLoad had a more descriptive name.

> Source/WebCore/loader/cache/CachedImage.h:116
> +    bool m_autoLoad;

What does this member variable mean? 
- "Autoload was enabled when object was created"
- "Autoload was enabled when making a decision whether to start loading"
- "Was loaded because of enabled autoload"
- "Started loading because autoload got enabled"
- "Did not try loading yet, so it's appropriate to start loading when autoload gets re-enabled"

ChangeLog says "whether [the resource] should be autoloaded", but which of the above options (if any) corresponds to that?

Even looking at this patch, I cannot easily tell what it is, and it will be nearly impossible to understand when looking at resulting ToT code.

[Ben Murdoch]

Thanks for the review Alexey, apologies for the confusion, please let me clarify:

> > Source/WebCore/loader/cache/CachedImage.h:116
> > +    bool m_autoLoad;
> 
> What does this member variable mean? 
> - "Autoload was enabled when object was created"
> - "Autoload was enabled when making a decision whether to start loading"
> - "Was loaded because of enabled autoload"
> - "Started loading because autoload got enabled"
> - "Did not try loading yet, so it's appropriate to start loading when autoload gets re-enabled"

It is used to indicate if the WebCore::Settings object is preventing us from loading the image. It's set when we first request the image and then updated when CachedResourceLoader::setAutoLoadImages gets executed (i.e. the setting is re-enabled). So probably closest to your second point. Agree a better name would make this clearer, how about m_autoLoadPrevented? Or even m_autoLoadPreventedBySettings to be even more explicit?

[Alexey Proskuryakov]

From this description, something like m_autoLoadWasPreventedBySettings or even m_loadWasPreventedBySettings sounds appropriate. I don't immediately see if this fully matches code changes, in particular this line doesn't simply set it to a value from settings:

resource->setAutoLoad(autoLoadImages() || (!resource->stillNeedsLoad() && resource->inCache()));

It will likely be easier to see in a patch that incorporates the more specific name.

[Ben Murdoch]

(In reply to comment #22)
> From this description, something like m_autoLoadWasPreventedBySettings or even m_loadWasPreventedBySettings sounds appropriate. I don't immediately see if this fully matches code changes, in particular this line doesn't simply set it to a value from settings:
> 
> resource->setAutoLoad(autoLoadImages() || (!resource->stillNeedsLoad() && resource->inCache()));

That is to ensure that we obey the semantics expressed in the documentation for the setting - that cached images should still be loaded even if autoloading images is turned off. I think I should be able to move the cache check into CachedImage where we check the new flag, then the flag will purely express the value from WebCore::Settings.

[Ben Murdoch]

Created attachment 116779 [details]
Patch.

I reversed the logic, hopefully it's still clear.

[Alexey Proskuryakov]

Comment on attachment 116779 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=116779&action=review

Thank you very much, the patch now appears clear enough for me to be able to review.

I have a number of comments that I expect would make you change the patch, so marking r-. Please feel free to mark for review again if you disagree with suggestions and implications from the questions.

> Source/WebCore/loader/cache/CachedImage.cpp:61
> +    , m_autoLoadPreventedBySettings(false)

I still think that "was" should be somewhere there for proper English grammar. "If auto load _was_ prevented by settings".

> Source/WebCore/loader/cache/CachedImage.cpp:121
> +    // If this image load is blocked, pretend we are loading now
> +    // so that we don't fire an onload event from CachedResource::didAddClient.

This is extremely subtle. Could CachedResource::didAddClient() call back into CachedImage instead, asking if it's appropriate to fire the event? Specifically, can it simply check if there is image data available, and skip firing the event if not?

That way, you would not need m_autoLoadPreventedBySettings at all.

> Source/WebCore/loader/cache/CachedImage.h:97
> +    void setAutoLoadPreventedBySettings(bool b) { m_autoLoadPreventedBySettings = b; }

Please use a real variable name instead of "b". Something like "prevented" would work.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:486
> +    bool shouldNotLoadNow = type == CachedResource::ImageResource && !autoLoadImages() && !inCache;

Safari preference is "Display images when the page opens".  Why does it matter whether the resource is in cache or not?

It's a pre-existing mismatch between wording in the UI and implementation, so you shouldn't be concerned about it. I'm just making this comment for posterity.

However, I'm surprised that you had to add the !inCache check. Why wasn't is needed before?

[Ben Murdoch]

Thanks Alexey, I think I will spin a new patch. Responses to your comments below:

> > Source/WebCore/loader/cache/CachedImage.cpp:61
> > +    , m_autoLoadPreventedBySettings(false)
> 
> I still think that "was" should be somewhere there for proper English grammar. "If auto load _was_ prevented by settings".
> 

OK, fair enough. I will add it.

> > Source/WebCore/loader/cache/CachedImage.cpp:121
> > +    // If this image load is blocked, pretend we are loading now
> > +    // so that we don't fire an onload event from CachedResource::didAddClient.
> 
> This is extremely subtle. Could CachedResource::didAddClient() call back into CachedImage instead, asking if it's appropriate to fire the event? Specifically, can it simply check if there is image data available, and skip firing the event if not?
> 
> That way, you would not need m_autoLoadPreventedBySettings at all.
> 

That's an interesting idea, I'll investigate.

> > Source/WebCore/loader/cache/CachedImage.h:97
> > +    void setAutoLoadPreventedBySettings(bool b) { m_autoLoadPreventedBySettings = b; }
> 
> Please use a real variable name instead of "b". Something like "prevented" would work.
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:486
> > +    bool shouldNotLoadNow = type == CachedResource::ImageResource && !autoLoadImages() && !inCache;
> 
> Safari preference is "Display images when the page opens".  Why does it matter whether the resource is in cache or not?

I was going by the documentation in WebCore/page/Settings.h which states:

// Unlike areImagesEnabled, this only suppresses the network load of
// the image URL.  A cached image will still be rendered if requested.

> 
> It's a pre-existing mismatch between wording in the UI and implementation, so you shouldn't be concerned about it. I'm just making this comment for posterity.
> 
> However, I'm surprised that you had to add the !inCache check. Why wasn't is needed before?

I think that's an existing bug where the implementation doesn't match the documentation in the header file. I could remove the cache check and the patch should still fix the original problem (but wouldn't load a cached image, naturally), but it would be good to update the header file in that case. I don't mind either way. What do you think?

[Alexey Proskuryakov]

> What do you think?

Let's limit this patch to fixing unload, and keep user observable behavior intact.

What I was asking about is that there was no explicit check for inCache in CachedResourceLoader::loadResource() before, but it was still loading resources from cache. So, it's unclear to me why an explicit check had to be added.

[Ben Murdoch]

(In reply to comment #27)
> > What do you think?
> 
> Let's limit this patch to fixing unload, and keep user observable behavior intact.
> 
> What I was asking about is that there was no explicit check for inCache in CachedResourceLoader::loadResource() before, but it was still loading resources from cache. So, it's unclear to me why an explicit check had to be added.

Ah, sorry - I thought you were referring to the other cache check. It seems that the one you were asking about is actually a relic from an old patch, and not needed.

So just to clarify - I will remove that one, but keep the cache check when determining whether to fire the onload event - to stick to the documentation in Settings.h and maintain existing behavior.

In my next patch I've changed it so that it's not necessary to toggle the setLoading flag to prevent the onload event being fired, I think it's neater. My personal preference is to keep the m_autoLoadWasPreventedBySettings flag as that makes it very explicit about the reasoning behind preventing the event being sent. I'm not sure if there are (or how many there may be now or in the future) overloaded meanings of an image not having image data available.

[Ben Murdoch]

Created attachment 117000 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 117000 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117000&action=review

> Source/WebCore/loader/cache/CachedImage.h:86
> +    bool stillNeedsLoad() const { return (!errorOccurred() && status() == Unknown && !isLoading()) || (m_autoLoadWasPreventedBySettings && !inCache()); }

I think that it's misleading to say that an image still needs load when settings say that we aren't loading any images.

[Ben Murdoch]

(In reply to comment #30)
> (From update of attachment 117000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117000&action=review
> 
> > Source/WebCore/loader/cache/CachedImage.h:86
> > +    bool stillNeedsLoad() const { return (!errorOccurred() && status() == Unknown && !isLoading()) || (m_autoLoadWasPreventedBySettings && !inCache()); }
> 
> I think that it's misleading to say that an image still needs load when settings say that we aren't loading any images.

I can see where you are coming from, but I don't fully agree. My thinking was simply that if an image hasn't yet been loaded (for whatever reason), then it should be considered as still needing loading. The user may toggle the setting for loading images at any time, at which point the image may become eligible for loading again. Perhaps s/Was/Is would be appropriate in the variable name, as we do update it in real time as the user makes changes to the setting.

Having said that, I'm happy to create a new function to capture this if you do not agree and would prefer that(can you suggest a name? isLoadingPrevented()?). It has to be on the CachedResource (unless inside CahedResource::didAddClient() I check if this is an image and then downcast to CachedImage, but that seems a little ugly). Adding this function is what I looked to do yesterday but it felt to me the stillNeedsLoad() captured what I wanted, so I used that.

[Mikhail Naganov]

Are there any updates on this issue? This affects embedded scenarios and we would like this bug to be fixed.

If the problem is only with the 'stillNeedsLoad' name, how about reversing its condition and renaming to 'loadingAttemptHasBeenMade'?

bool loadingAttemptHasBeenMade() const { 
  return (errorOccurred() || status() != Unknown || isLoading() || inCache()) &&
         !m_autoLoadWasPreventedBySettings;
}

[Mikhail Naganov]

The issue was resolved in https://bugs.webkit.org/show_bug.cgi?id=95478

*** This bug has been marked as a duplicate of bug 95478 ***

[Eric Seidel (no email)]

Comment on attachment 117000 [details]
Patch

Cleared review? from attachment 117000 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).