web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.86k stars 3.03k forks source link

COEP tests where iframe fails to load #21300

Closed annevk closed 4 years ago

annevk commented 4 years ago

@ParisMeuleman @ArthurSonzogni html/cross-origin-embedder-policy/require-corp-about-blank.html and equivalent tests rely on the iframe element firing a load event when there's a network error. There's currently no agreement on that being the right thing: https://github.com/whatwg/html/issues/125 (there's various issues around this).

We need to fix these tests to not rely on that until there's agreement on that behavior.

Also, when you use promise_test you do need t.step_func wrappers for any event handling as otherwise exceptions fired as part of those events end up failing the test harness as is happening in Firefox. That's problematic. Documentation on this was wrong, but is now corrected at https://web-platform-tests.org/writing-tests/testharness-api.html#promise-tests.

ParisMeuleman commented 4 years ago

Would replacing let iframe_B_loaded = new Promise(resolve => iframe_B.onload = resolve); by let iframe_B_loaded = new Promise(resolve => { iframe_B.onload = resolve; iframe_B.onerror = resolve; }); make the trick?

Arthur made the following script yesterday and we saw the difference in behavior between FF and CR:

let w = window.open();
let iframe = w.document.createElement("iframe");
iframe.src = "https://error-page"
iframe.onerror = () => console.log("onerror")
iframe.onload = () => console.log("onload")
w.document.body.appendChild(iframe);
annevk commented 4 years ago

No, you need to wait for a timeout. And it would be better not to do anything with load as per the specification it's not expected to be dispatched at this point. (Adding a comment pointing to the whatwg/html issue might be good though, so we can clean this up once that gets resolved.)

ParisMeuleman commented 4 years ago

Indeed onerror is not always called! thanks.

I have a prototype that I think works:

waitForFrameLoadedOrBlocked = (frame) => {
  return new Promise(resolve => {
    let interval;
    done = () => {
      clearInterval(interval);
      resolve();
    }
    interval = setInterval(() => {
      try{ frame.contentWindow.a } catch(e) {
        done(interval, resolve);
      }
    }, 1);
    frame.onload = done;
  });
}
[...]
let iframe_B_loaded = waitForFrameLoadedOrBlocked(iframe_B);
let iframe_C_loaded = waitForFrameLoadedOrBlocked(iframe_C);

onload is kept here, supposing that we accept both a load or an error for the frame, which allows for the completion of test even if COEP was mishandled.

annevk commented 4 years ago

I'd rather only use load if we expect it to not result in a network error. And the precedent in other tests is to use t.step_timeout with 500/1500 as timeout. Using interval might be interesting to let it resolve more quickly, but I think we also want an ultimate timeout for individual tests and not let those result in a harness failure.