1264768 - Image loads for URLs that fail to resolve don't dispatch error event

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

According to the specification (https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data), in step 11.4 we should be dispatching an error event.

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4060

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Dispatch error event when image loads for URLs that fail to resolve

WIP patch that passes comment 0 test. Will verify flow based on spec and run try.

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Dispatch error event when image loads for URLs that fail to resolve

Change:
- dispatch error event as long as method |StringToURI| fails.

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1 (v3): Dispatch error event when image loads for URLs that fail to resolve

Change
- dispatch error event AFTER canceling image requests, per comment 0 spec
- add some comment to clarify

Comment 4

[Ben Tian (inactive)]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aab607f59038

Comment 3 patch seems irrelevant to the failed test cases (all copy/paste related on Win7).

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8757799 [details] [diff] [review]
Patch 1 (v3): Dispatch error event when image loads for URLs that fail to resolve

Henri,

Can you review my patch that dispatches error event when image loads for URLs that fail to resolve? The patch is based on comment 0 spec and passes comment 0 test. Try result is in comment 4.

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 8757799 [details] [diff] [review]
Patch 1 (v3): Dispatch error event when image loads for URLs that fail to resolve

Review of attachment 8757799 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsImageLoadingContent.cpp
@@ +755,5 @@
> +  if (NS_FAILED(rv)) {
> +    // Cancel image requests and fire error event per spec
> +    CancelImageRequests(aNotify);
> +    FireEvent(NS_LITERAL_STRING("error"));
> +    return rv;

Why is rv returned instead of NS_OK? The other similar cases where "error" is fired, NS_OK is returned.

Comment 7

[Ben Tian (inactive)]

(In reply to Henri Sivonen (:hsivonen) from comment #6)
> Why is rv returned instead of NS_OK? The other similar cases where "error"
> is fired, NS_OK is returned.

I just want to keep original behavior that returns rv in NS_ENSURE_SUCCESS, in case any existing caller expects rv returned. I'm fine to return NS_OK if you suggest.

Comment 8

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian][PTO: 6/20 ~ 7/1] from comment #7)
> I just want to keep original behavior that returns rv in NS_ENSURE_SUCCESS,
> in case any existing caller expects rv returned. I'm fine to return NS_OK if
> you suggest.

If NS_OK is returned here, both |LoadImage| methods can return void instead for always returning NS_OK. And so does method |FireEvent|. I can open another bug to clean these up.

Comment 9

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian][PTO: 6/18 ~ 7/3] from comment #7)
> (In reply to Henri Sivonen (:hsivonen) from comment #6)
> > Why is rv returned instead of NS_OK? The other similar cases where "error"
> > is fired, NS_OK is returned.
> 
> I just want to keep original behavior that returns rv in NS_ENSURE_SUCCESS,
> in case any existing caller expects rv returned. I'm fine to return NS_OK if
> you suggest.

Henri, do you suggest to return NS_OK instead? Let me know for any other suggestion on my patch.

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Ben Tian [:btian][PTO: 6/18 ~ 7/3] from comment #9)
> (In reply to Ben Tian [:btian][PTO: 6/18 ~ 7/3] from comment #7)
> > (In reply to Henri Sivonen (:hsivonen) from comment #6)
> > > Why is rv returned instead of NS_OK? The other similar cases where "error"
> > > is fired, NS_OK is returned.
> > 
> > I just want to keep original behavior that returns rv in NS_ENSURE_SUCCESS,
> > in case any existing caller expects rv returned. I'm fine to return NS_OK if
> > you suggest.
> 
> Henri, do you suggest to return NS_OK instead? Let me know for any other
> suggestion on my patch.

Based on an MXR search, yes, I'm suggesting return NS_OK instead.

Comment 11

[Henri Sivonen (:hsivonen)]

Comment on attachment 8757799 [details] [diff] [review]
Patch 1 (v3): Dispatch error event when image loads for URLs that fail to resolve

r+ if the return value is changed to NS_OK.

Comment 12

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Dispatch error event when image loads for URLs that fail to resolve, r=hsivonen

Thanks for the review! Revise per reviewer's comment.

Comment 13

[Ben Tian (inactive)]

Try result of comment 12 patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73914b2ea3e4

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/066be6391756
Dispatch error event when image loads for URLs that fail to resolve, r=hsivonen

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/066be6391756

Comment 16

[Edgar Chen [:edgar]]

This cause a unexpected error event in non-responsive mode, see bug 1321300 comment #7.
It is because we load image for each src attribute assignment if in non-responsive mode. Bug 1076583 will fix that (load image from last associated src value only). However, bug 1076583 still has ongoning work and may not be landed soon, so we consider back out this change first (at least we have consistent behavior as 49 and earlier versions) and reland it after bug 1076583 is fixed.