Going to disable for now.

A number of fixes wrapped up in this patch: * Enable correct prefs * Use HTTPS origins to avoid unintended mixed content checks. I believe this was an error from the http-to-https translation when importing the tests. * Treat our error pages as cross origin. This required building a new helper function because cross origin windows cannot even be passed through Promise.resolve(). Instead we coerce the error window into a "content" string before resolving the promise. * Updated the CORS response tests to load the window. This is a change in the spec that both chrome and firefox still need to implement. Bug listed in the ini.

Also, I know I didn't implement the check against the expected body. I kept the empty string comparisons. I guess I don't see the value in reworking that in the test. It would be either very verbose or require substring matching or something similarly sketchy. The "not empty string" comparisons seem equally valid for these tests.

Actually, it was not so bad to examine the returned results as it was already formatted in JSON (after a fashion). I updated the "window will load" tests to expect the window content to parse as JSON with a particular success property set.

Comment on attachment 8641386 [details] [diff] [review] Fix bad origins, error page handling, and spec issues in fetch-frame-resources.https.html. r=jgraham Review of attachment 8641386 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, essentially. Just a couple of small issues, and some optional things. ::: testing/web-platform/mozilla/meta/service-workers/service-worker/fetch-frame-resource.https.html.ini @@ +1,3 @@ > [fetch-frame-resource.https.html] > type: testharness > + prefs: [dom.serviceWorkers.enabled: true, Should this go in a __dir__.ini file so it's enabled for all tests here? ::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-frame-resource.https.html @@ +22,5 @@ > + // Eval the returned string with a report functionto get the json > + // object. > + try { > + function report(obj) { result = obj }; > + eval(contentString); Could we change the script file not to wrap with report() and result = JSON.parse(contentString)? Don't worry if that means changing too many places. @@ +31,5 @@ > + resolve(result); > + } > + > + // We can't catch the network error on window. So we use the timer. > + var timeout = setTimeout(function() { Using a timer here makes me nervous about intermittents, but if we are trying to test that something didn't happen, I guess we might not have too much choice. @@ +43,5 @@ > + content = contentFunc(win); > + } catch(e) { > + // use default empty string for cross-domain window > + } > + done(content); I wonder why we don't return a string like '{"jsonpResult": "timeout"}' here @@ +58,5 @@ > +function getLoadedFrameAsObject(frame) { > + return getLoadedObject(frame, function(f) { > + return f.contentDocument.body.textContent; > + }, function(f) { > + f.remove(); Is .remove() well supported? Would f.parentNode().removeChild(f) work just as well? @@ +118,4 @@ > return service_worker_unregister_and_done(t, scope); > }) > .catch(unreached_rejection(t)); > + }, 'CORS type response could be loaded in the iframe.'); I assume it's intended that the pass condition here revered?

(In reply to James Graham [:jgraham] from comment #7) > testing/web-platform/mozilla/tests/service-workers/service-worker/fetch- > frame-resource.https.html > @@ +22,5 @@ > > + // Eval the returned string with a report functionto get the json > > + // object. > > + try { > > + function report(obj) { result = obj }; > > + eval(contentString); > > Could we change the script file not to wrap with report() and > result = JSON.parse(contentString)? > Don't worry if that means changing too many places. This .py file is used in a number of places in the test. I was trying to avoid rabbit holing on a bunch of other changes right now. > @@ +31,5 @@ > > + resolve(result); > > + } > > + > > + // We can't catch the network error on window. So we use the timer. > > + var timeout = setTimeout(function() { > > Using a timer here makes me nervous about intermittents, but if we are > trying to test that something didn't happen, I guess we might not have too > much choice. The setTimeout() was existing in the test. Its the only way to detect if a window did not load, unfortunately. And onerror does not fire in all browsers for iframe. Yes, it sucks. I don't have a better solution. > @@ +43,5 @@ > > + content = contentFunc(win); > > + } catch(e) { > > + // use default empty string for cross-domain window > > + } > > + done(content); > > I wonder why we don't return a string like '{"jsonpResult": "timeout"}' here I'd rather not do this. I think the whole idea is that the JSON.parse() is a reasonable way to positively tell that we got a real response from the fetch. If we coerce to another JSON value then I think it muddies the test a bit. Keep in mind, we might not manually modify the content string at all here and use whatever the browser gives us if its not treating the window as cross-origin. > @@ +58,5 @@ > > +function getLoadedFrameAsObject(frame) { > > + return getLoadedObject(frame, function(f) { > > + return f.contentDocument.body.textContent; > > + }, function(f) { > > + f.remove(); > > Is .remove() well supported? Would f.parentNode().removeChild(f) work just > as well? MDN suggests its only in blink/gecko. I'll switch. > @@ +118,4 @@ > > return service_worker_unregister_and_done(t, scope); > > }) > > .catch(unreached_rejection(t)); > > + }, 'CORS type response could be loaded in the iframe.'); > > I assume it's intended that the pass condition here revered? Yes. The spec changed to allow CORS responses. Our implementations need to catch up, but I thought I should correct the spec now.

Updated based on review feedback. I ran the entire test suite locally and the __dir__.ini prefs didn't break other tests.

Forgot to hg add the __dir__.ini file.

Try showed the 1 second timeout was not long enough on debug builds in automation. Particularly when the browser crashed in a previous test and cache/jit is cold. Here is a try with 5 second timeout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=204df250c14e