Bug 215998 – Fix crash in image-loading-lazy-slow.html WPT test

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Rob Buis]

Make image-loading-lazy-slow.html pass. Note that this was not tested before because it had no custom webkit-test-runner directive to enable lazy image loading.

[Rob Buis]

Created attachment 407961 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 407968 [details]
Patch

[youenn fablet]

Comment on attachment 407968 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:280
> +            if (document.lazyLoadImageObserver().isObserved(element()))

Do we need this if check?

> Source/WebCore/loader/ImageLoader.cpp:281
> +                LazyLoadImageObserver::unobserve(element(), element().document());

This is ok as is, but it would be a bit better if it was reverse so that we unobserve closer to setting m_image.
Something like:
if (!m_image)
   unobserve....

There are other code paths that sets back m_image to nullptr.
Should we unobserve in those cases as well?

Maybe we should introduce a clearImage() method that would set back m_image to nullptr and unobserve.

[Alexey Proskuryakov]

Comment on attachment 407968 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Mark image-loading-lazy-slow.html as ImageOnlyFailure.

So this patch doesn't do what its title says?

[Rob Buis]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 407968 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407968&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Mark image-loading-lazy-slow.html as ImageOnlyFailure.
> 
> So this patch doesn't do what its title says?

You are right, I was expecting the test would pass once the crash is fixed, but with the crash fixed we run into https://bugs.webkit.org/show_bug.cgi?id=186045.

[Rob Buis]

Created attachment 408165 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/68454263>

[Rob Buis]

Comment on attachment 407968 [details]
Patch

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

>> Source/WebCore/loader/ImageLoader.cpp:280
>> +            if (document.lazyLoadImageObserver().isObserved(element()))
> 
> Do we need this if check?

Right, that one was a bit ugly, I changed isDeferred to include testing for LoadImmediate, and instead of above check check for isDeferred.

>> Source/WebCore/loader/ImageLoader.cpp:281
>> +                LazyLoadImageObserver::unobserve(element(), element().document());
> 
> This is ok as is, but it would be a bit better if it was reverse so that we unobserve closer to setting m_image.
> Something like:
> if (!m_image)
>    unobserve....
> 
> There are other code paths that sets back m_image to nullptr.
> Should we unobserve in those cases as well?
> 
> Maybe we should introduce a clearImage() method that would set back m_image to nullptr and unobserve.

I think in the other places we do not need to unobserve. But I added a method resetLazyImageLoading which we can re-use if needed.

[EWS]

Committed r266720: <https://trac.webkit.org/changeset/266720>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408165 [details].