whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.05k stars 2.64k forks source link

Chrome/Edge clears pending error event if img.src changed before it fired #1872

Open EdgarChen opened 7 years ago

EdgarChen commented 7 years ago

Chrome/Edge seems clear pending error event if img.src changed before it fired.

Example 1:

var image1 = new Image();
var image2 = new Image();

image1.src = '';
image2.src = '';

image1.onerror = function() {
  console.log('image1.onerror');
  image2.src = 'data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="96" height="96"><path d="M10,10L32,90L90,32z" fill="lightgreen"/></svg>';
};

image2.onerror = function() {
  console.log('image2.onerror');
};

Result of Chrome and Edge:     Image2's error event isn't fired.
Example 2:

var image1 = new Image();
var image2 = new Image();

image1.src = '';
image2.src = '';

image1.onerror = function() {
  console.log('image1.onerror');
  image2.src = '';
};

image2.onerror = function() {
  console.log('image2.onerror');
};

Result of Chrome and Edge:     Image2's error event is only fired once.
This behavior (clear pending error event) isn't documented in the spec. But some sites seems rely on this behavior, e.g. https://www.bing.com/mapspreview [1]. Does this behavior is what we expect?

[1] Please also see https://bugzilla.mozilla.org/show_bug.cgi?id=1308069

domenic commented 7 years ago

@zcorpan is our expert here, and I think is on vacation for a week. But are you sure this behavior isn't documented by the spec? It might not happen by "clearing" the error event, but by the queued task checking that the error condition is still present before it fires.

There are several places that fire error events, so I might not have this particular scenario right, but at a quick skim, "If the resource type and data corresponds to a supported image format, as described below" might be the section you're talking about, and it specifically says "and image request is pending request" before firing the error event (so it won't be fired if the src has been changed, from what I understand).

EdgarChen commented 7 years ago

The error event of my example is from step 9:

9). If selected source is null, run these substeps:     1). Set the current request to the broken state, abort the image request for the current request and the pending request, and let pending request be null.     2). Queue a task to change the current request's current URL to the empty string, and then, if the element has a src attribute or it uses srcset or picture, fire a simple event named error at the img element.     3). Abort this algorithm.

If we set src to empty string, spec says "Queue a task to fire a simple event named error". And before the event task has been ran, we changed the src. From what I understand, the task will still be ran, and the error event will be fired as well. But my examples show different behavior in Bink and Edge. Or do I misunderstand any thing? Thank you.

bzbarsky commented 7 years ago

Stepping carefully though the spec for this testcase:

  1. We enter https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data for image1.
  2. We proceed until step 6, then await a stable state.
  3. We enter https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data for image 2.
  4. We proceed until step 6, then await a stable state.
  5. Stable state is reached, we start with step 7 for image1 (stable state stuff is ordered, though this is not immediately obvious).
  6. We reach step 9 for image1, queue a task to fire an error event at image1, and abort the algorithm.
  7. We start with step 7 for image2, reach step 9 for image2, queue a task to fire an error event on image2.
  8. Error event for image1 fires, reenters https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data for image2. But none of this affects the task that was queued in step 7 of this list.

Note also that the spec's task will munge the current request's current URL, even if a new update has started and there is a new, totally valid, current request. So yes, the spec seems kinda broken here.

bzbarsky commented 7 years ago

And yes, I think the right fix is for the task queued in https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data step 9.2 to check the "If another instance of this algorithm for this img element was started after this instance" thing....

bzbarsky commented 7 years ago

And maybe other steps like 11.4, 5.3.7, and maybe parts of step 14. Step 14 is hard to test reliably, but the other two are trivial to test using a testcase like the one above. Should check what various UAs do currently.