Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

img.complete behavior is wrong when a new load is started for the same image #4884

Open
bzbarsky opened this issue Sep 3, 2019 · 6 comments

Comments

@bzbarsky
Copy link
Contributor

bzbarsky commented Sep 3, 2019

Consider this testcase:

  var img = new Image();
  img.src = "url1";
  img.onload = () => {
    img.src = "url2"; // Make sure this is not in the document's loaded-images cache
    alert(img.complete);
  }

Per spec at https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-complete this should alert true because:

  • First bullet does not apply, because there is an src attribute
  • Second bullet does not apply, because src is nonempty
  • Third bullet, who knows
  • Fourth bullet applies (!) because there is a current request and its status is completely available.

Presumably complete should returns false if there is a pending request or something? It consistently returns false in browsers on the above testcase.

@EdgarChen @annevk @domenic

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Sep 3, 2019

Once this is fixed, the comment in the img.complete.html WPT referencing this issue should be removed.

@EdgarChen
Copy link
Member

Presumably complete should returns false if there is a pending request or something? It consistently returns false in browsers on the above testcase.

Checking pending request seems not help here, nor does #4476, given that the steps after step 7 of update-image-data is async per spec?

Current Gecko returns false is because we run update-image-data in a sync way for non-responsive case in Gekco implementation. I suspect other browsers also run update-image-data in a sync way for this case.

Another interesting example,

var img = new Image();
  img.srcset = "url1";
  img.onload = () => {
    img.srcset = "url2"; // Make sure this is not in the document's loaded-images cache
    alert(img.complete);
  }

Gecko returns true in this example, it is because we do follow spec that run update-image-data in an async way for responsive case. However, other browsers still return false, It looks like that other browsers still run update-image-data in a sync way even for responsive case?

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Sep 4, 2019

At least in Chrome, .complete returns false if the "update the image data" task is queued at all (and they queue it in all sorts of cases when they should not; see https://bugs.chromium.org/p/chromium/issues/detail?id=1000273).

It would make some sense to return false for .complete if we have reached step 7 of the update the image data steps, I suspect... as long as we don't do it in the cases that would return early in step 10 and maybe step 11? Unclear.

@annevk

This comment has been minimized.

@bzbarsky
Copy link
Contributor Author

This seems like a duplicate of #4475.

That claims to be resolved by #4934 but this issue is still unresolved, as far as I can tell. See #4934 (comment)

@jdm

@annevk
Copy link
Member

annevk commented Oct 22, 2019

I think I see now, the problem is that https://html.spec.whatwg.org/#update-the-image-data does not update any state on the image element synchronously when an asynchronous fetch is to be performed and therefore complete might still return the wrong value as it only uses such state.

I think this is related to some other problems with the img element about it being unclear whether it should use "await a stable state". Parsing that URL that late is also somewhat problematic from a blob URL perspective, for instance. (#2429 as @EdgarChen referenced already.)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 5, 2024
…iewers,devtools-reviewers,robwu,nchevobbe

This fixes src loads to be consistent with srcset/picture loads, modulo
the special synchronous case in the spec
(https://html.spec.whatwg.org/#update-the-image-data step 7), which
requires src loads to be sync if the image is available.

We now avoid triggering the load from the parser consistently for src /
srcset / picture, and unify the codepath with BindToTree. That should
avoid some useless task allocations.

Only the sync load code-path needs a script runner (mostly to deal with
anonymous content like the video poster <img> and such, but it also
helps not trigger sync loads at unexpected times like on adoption).

About the HTMLImageElement::Complete() getter change, we need to also
return false if there's an existing load task. That is the proposal in
whatwg/html#4884, and prevents some failures
in the-img-element/{update-src-complete,img.complete}.html WPTs. It
technically changes our behavior on .srcset changes, but it makes it
consistent with .src changes and other browsers, so seems fine.

There are a couple regressions in CSP tests and the networkEvent stubs,
but these are really a pre-existing issue. What happens is that, since
the loads are now async, CSP can't figure out the script that triggered
the load anymore. I need to look if there's an easy way to propagate
that information in the image load tasks, but this is trivially
reproducible by changing these tests to use srcset rather than src.

The rest of the test changes are as expected: either new passes, or
expected test changes from this.

Differential Revision: https://phabricator.services.mozilla.com/D215519
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Aug 6, 2024
…iewers,devtools-reviewers,robwu,nchevobbe

This fixes src loads to be consistent with srcset/picture loads, modulo
the special synchronous case in the spec
(https://html.spec.whatwg.org/#update-the-image-data step 7), which
requires src loads to be sync if the image is available.

We now avoid triggering the load from the parser consistently for src /
srcset / picture, and unify the codepath with BindToTree. That should
avoid some useless task allocations.

Only the sync load code-path needs a script runner (mostly to deal with
anonymous content like the video poster <img> and such, but it also
helps not trigger sync loads at unexpected times like on adoption).

About the HTMLImageElement::Complete() getter change, we need to also
return false if there's an existing load task. That is the proposal in
whatwg/html#4884, and prevents some failures
in the-img-element/{update-src-complete,img.complete}.html WPTs. It
technically changes our behavior on .srcset changes, but it makes it
consistent with .src changes and other browsers, so seems fine.

There are a couple regressions in CSP tests and the networkEvent stubs,
but these are really a pre-existing issue. What happens is that, since
the loads are now async, CSP can't figure out the script that triggered
the load anymore. I need to look if there's an easy way to propagate
that information in the image load tasks, but this is trivially
reproducible by changing these tests to use srcset rather than src.

The rest of the test changes are as expected: either new passes, or
expected test changes from this.

Differential Revision: https://phabricator.services.mozilla.com/D215519
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Aug 7, 2024
…iewers,devtools-reviewers,robwu,nchevobbe

This fixes src loads to be consistent with srcset/picture loads, modulo
the special synchronous case in the spec
(https://html.spec.whatwg.org/#update-the-image-data step 7), which
requires src loads to be sync if the image is available.

We now avoid triggering the load from the parser consistently for src /
srcset / picture, and unify the codepath with BindToTree. That should
avoid some useless task allocations.

Only the sync load code-path needs a script runner (mostly to deal with
anonymous content like the video poster <img> and such, but it also
helps not trigger sync loads at unexpected times like on adoption).

About the HTMLImageElement::Complete() getter change, we need to also
return false if there's an existing load task. That is the proposal in
whatwg/html#4884, and prevents some failures
in the-img-element/{update-src-complete,img.complete}.html WPTs. It
technically changes our behavior on .srcset changes, but it makes it
consistent with .src changes and other browsers, so seems fine.

There are a couple regressions in CSP tests and the networkEvent stubs,
but these are really a pre-existing issue. What happens is that, since
the loads are now async, CSP can't figure out the script that triggered
the load anymore. I need to look if there's an easy way to propagate
that information in the image load tasks, but this is trivially
reproducible by changing these tests to use srcset rather than src.

The rest of the test changes are as expected: either new passes, or
expected test changes from this.

Differential Revision: https://phabricator.services.mozilla.com/D215519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants