| Summary: | Fix crash in image-loading-lazy-slow.html WPT test | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rob Buis <rbuis> | ||||||||
| Component: | Images | Assignee: | Rob Buis <rbuis> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, changseok, clopez, esprehn+autocc, ews-watchlist, gyuyoung.kim, japhet, webkit-bug-importer, youennf | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Rob Buis
2020-08-31 02:41:03 PDT
Created attachment 407961 [details]
Patch
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 Created attachment 407968 [details]
Patch
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. 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? (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. Created attachment 408165 [details]
Patch
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. Committed r266720: <https://trac.webkit.org/changeset/266720> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408165 [details]. |