Bug 266676 - Don't error out if we successfully loaded an image
Summary: Don't error out if we successfully loaded an image
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: BrowserCompat, GoodFirstBug, InRadar
Depends on:
Blocks:
 
Reported: 2023-12-19 18:31 PST by Ahmad Saleem
Modified: 2024-03-04 08:38 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ahmad Saleem 2023-12-19 18:31:20 PST
Hi Team,

While going through Blink's commit, I came across one potential merge:

Blink Commit: https://chromium.googlesource.com/chromium/blink/+/c1fdb4cd2099a27ae10dba405fe2869121c50545

"The spec shows that this behaviour is OK for the object element but not for the
img element, where its silence implies we shoud load and display the image if we
got one - even if the response was a 404."

WebKit Source: https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLImageLoader.cpp#83

Chromium / Blink still has modified test case.

Just wanted to raise to get input, so we can fix it (if needed).

Thanks!
Comment 1 Ahmad Saleem 2023-12-19 18:36:14 PST
For <object> - Due to the algorithm above, the contents of object elements act as fallback content, used only when referenced resources can't be shown (e.g. because it returned a 404 error.

https://html.spec.whatwg.org/#the-object-element

Can't find anything about 'img' element. CCing - @Anne (to check for his expertise on web-spec).
Comment 2 Anne van Kesteren 2023-12-19 23:30:45 PST
That code looks wrong indeed. We should not fail to load an image due to an HTTP status code.

I thought WPT had coverage for this as well, but maybe not?
Comment 3 Karl Dubost 2023-12-20 04:42:58 PST
Yup and that should be an easy fix.
Comment 4 Ahmad Saleem 2023-12-20 06:26:06 PST
PR - https://github.com/WebKit/WebKit/pull/22100
Comment 5 Ahmad Saleem 2023-12-20 06:32:41 PST
We do have this WPT - http://wpt.live/html/semantics/embedded-content/the-img-element/404-response-with-actual-image-data.html

and we are passing it. :-?
Comment 6 Radar WebKit Bug Importer 2023-12-26 18:32:14 PST
<rdar://problem/120188957>
Comment 7 Ahmad Saleem 2024-03-04 08:23:45 PST
Reading code bit better - I agree with Chris's comment on PR:

"I don't think this code change actually causes any web facing behavior change? The chromium code change made more sense because loadError was used for various element types.

Here the only thing you changed is whether we report the extra memory to the GC or not it seems."

---

Plus we are already passing WPT test, so we don't need to do this.

Only benefit - we can take from this commit would be to update 'test' to 'ref-test'. Although I am in favor of deleting it since we have WPT test coverage.

@Anne & @Karl - any input, I am closing my PR but I can use this bug to delete local test. :-)
Comment 8 Anne van Kesteren 2024-03-04 08:38:32 PST
Removing local tests that are (near) identical to imported WPT tests seems fine.

I don't understand why our code still has a 400 check in it though. Even with your change that was kept, but I believe the HTML Standard has no such check for image fetches.