We should add a test case for tor broswer for Regression tests for blob URL isolation. https://torpat.ch/15502

Still lack worker_blobify and worker_deblobify.

Added worker_blobify and worker_deblobify. I'll ask for review after adding the media source tests as Arthur suggested last week to also put them in this patch.

As I also started to write media source url tests, I don't find much to reuse, so I think putting them in separate tests should be find. Arthur, please take a look. Thanks.

Comment on attachment 8786594 [details] [diff] [review] Test blob url isolation by origin attributes. Review of attachment 8786594 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! r+ with changes below. Also for our funding purposes it would be nice to have an acknowledgement in the commit message, "Adapted from Tor Browser patch 15502". ::: browser/components/originattributes/test/browser/browser_blobURLIsolation.js @@ +66,5 @@ > +} > + > +function* worker_blobify(browser, input) { > + return yield* workerIO(browser, SCRIPT_WORKER_BLOBIFY, input); > +} This can be `let worker_blobify = (browser, input) => workerIO(browser, SCRIPT_WORKER_BLOBIFY, input);` @@ +70,5 @@ > +} > + > +function* worker_deblobify(browser, blobURL) { > + return yield* workerIO(browser, SCRIPT_WORKER_DEBLOBIFY, blobURL); > +} Similar. @@ +75,5 @@ > + > +let blobURL; > +function doTest(blobify, deblobify) { > + return function* (browser, tabNum) { > + if (tabNum == 0) { As we did with localStorage, let's not use a tabNum argument here. Instead use `if (blobURL === undefined)`. @@ +80,5 @@ > + let input = Math.random().toString(); > + blobURL = yield* blobify(browser, input); > + return input; > + } > + return yield* deblobify(browser, blobURL); I think you can use yield rather than yield*. ::: browser/components/originattributes/test/browser/head.js @@ +304,5 @@ > secondFrameSetting); > > // Fetch results from tabs. > + let resultA = yield aGetResultFunc(tabInfoA.browser, 0); > + let resultB = yield aGetResultFunc(tabInfoB.browser, 1); Don't add the second argument here.

Thanks, Arthur. Hi Baku, could you also review this patch?

Comment on attachment 8787125 [details] [diff] [review] Blob url isolation tests, adapted from Tor Browser patch 15502. Review of attachment 8787125 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/originattributes/test/browser/worker_blobify.js @@ +3,5 @@ > +// post back a blob URL pointing to the blob. > +self.addEventListener("message", function (e) { > + try { > + var blob = new Blob([e.data]), > + blobURL = URL.createObjectURL(blob); Just because these 2 variables are sequentially, I think it's clear if you do: var blob = new Blob([e.data]); var blobURL = URL.createObjectURL(blob); or directly: var blobURL = URL.createObjectUrl(new Blob([e.data])) @@ +4,5 @@ > +self.addEventListener("message", function (e) { > + try { > + var blob = new Blob([e.data]), > + blobURL = URL.createObjectURL(blob); > + postMessage(blobURL); just to differentiate blobURL and error messages, what about if you do: postMessage({ blobURL }); @@ +6,5 @@ > + var blob = new Blob([e.data]), > + blobURL = URL.createObjectURL(blob); > + postMessage(blobURL); > + } catch (e) { > + postMessage(e.message); and here: postMessage({error: e.message}); ::: browser/components/originattributes/test/browser/worker_deblobify.js @@ +3,5 @@ > +// Post back the string. > + > +var postStringInBlob = function (blobObject) { > + var fileReader = new FileReaderSync(), > + result = fileReader.readAsText(blobObject); same here. @@ +9,5 @@ > +}; > + > +self.addEventListener("message", function (e) { > + var blobURL = e.data, > + xhr = new XMLHttpRequest(); Here you should do: if ("error" in e.data) { postMessage(e.data); return; } @@ +21,5 @@ > + }; > + xhr.responseType = "blob"; > + xhr.send(); > + } catch (e) { > + postMessage(e.message); {error: e.message})

Thanks, Baku. I fixed them. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6d95a103bf

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dfb2d13513f Blob url isolation tests, adapted from Tor Browser patch 15502. r=baku, r=arthuredelstein

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/160a02f0a460 Backed out changeset 4dfb2d13513f because it blocks the backout of bug 1260931. r=backout on a CLOSED TREE

Had to back this out to properly backout bug 1260931: https://hg.mozilla.org/integration/mozilla-inbound/rev/160a02f0a46091f341d2d937167963e4645bba67

This also failed its test in the push which followed the landing. Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=35209852&repo=mozilla-inbound

Arthur, do you think we can just do (page_blobify, page_deblobify) and (worker_blobify, worker_deblobify) instead of the four combinations to save time? Otherwise we'll have to requestLongerTimeout.

(In reply to Jonathan Hao [:jhao] from comment #13) > Arthur, do you think we can just do (page_blobify, page_deblobify) and > (worker_blobify, worker_deblobify) instead of the four combinations to save > time? Otherwise we'll have to requestLongerTimeout. The reason I did it that way is that I wanted to ensure that the isolation code was consistent between pages and workers. And this is going to take even longer once we activate first-party isolation, right? So I would be inclined to requestLongerTimeout. Or maybe we can figure out a way to speed up the whole isolation testing framework? One thought I have is that maybe it would help to do more things in parallel. For example, in browser/components/originattributes/test/browser/head.js, currently we yield on the creation of each tab: + let tabInfoA = yield IsolationTestTools._addTab(aMode, + pageURL, + tabSettings[tabSettingA], + firstFrameSetting); + let tabInfoB = yield IsolationTestTools._addTab(aMode, + pageURL, + tabSettings[tabSettingB], + secondFrameSetting); But instead we could do + let tabInfoAPromise = IsolationTestTools._addTab(aMode, + pageURL, + tabSettings[tabSettingA], + firstFrameSetting); + let tabInfoBPromise = IsolationTestTools._addTab(aMode, + pageURL, + tabSettings[tabSettingB], + secondFrameSetting); and then + let [tabInfoA, tabInfoB] = yield Promise.all([tabInfoAPromise, tabInfoBPromise]); and the same approach could be used for fetching results and closing tabs. Does this speed it up significantly? If so, I could imagine parallelizing even further by allowing the framework to accept a list of test functions like IsolationTestTools.runTestsInParallel(TEST_PAGE, [doTest(page_blobify, page_deblobify), doTest(worker_blobify, page_deblobify), doTest(page_blobify, worker_deblobify), doTest(worker_blobify, page_deblobify), ]); and then the framework could run the tests using Promise.all.

(In reply to Jonathan Hao [:jhao] from comment #8) > Created attachment 8787535 [details] [diff] [review] > Blob url isolation tests, adapted from Tor Browser patch 15502. > > try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6d95a103bf I noticed something that should be fixed in the latest version of the patch: > +let blobURL = null; > +function doTest(blobify, deblobify) { > + return function* (browser, tabNum) { > + if (blobURL === null) { should be +function doTest(blobify, deblobify) { + let blobURL = null; + return function* (browser) { + if (blobURL === null) { There two changes: the tabNum argument has been removed, and `let blobURL = null` goes inside doTest. That way we get a new blobURL created for each doTest.

Thanks, Arthur. I fixed them.

Another possible way to speed up the framework is to simply open two tabs at the beginning (tab A and tab B), and then for each call to IsolationTestTools.runTest(...), simply change the location of the two tabs to the new desired pages (page A and page B). That way we don't have to wait for each tab to be created and destroyed.

I tried opening/closing two tabs in parallel, but I can't observe any speed up locally. We can't fetch results in parallel because we need the result (blob URL) from the first tab before we spawn the task to the second tab. The performance may still improve if we try running four tests in parallel, because there are things done in the worker thread.

Since creating and retrieving blob urls doesn't affect each other. I let the four combinations reuse the opened tabs, instead of opening new tabs for each combinations. This reduce the running time from 25s to 17s on my laptop. I think this is enough to pass original timeout. Arthur and Baku, please take a look again. Thanks.

(In reply to Arthur Edelstein [:arthuredelstein] from comment #17) > Another possible way to speed up the framework is to simply open two tabs at > the beginning (tab A and tab B), and then for each call to > IsolationTestTools.runTest(...), simply change the location of the two tabs > to the new desired pages (page A and page B). That way we don't have to wait > for each tab to be created and destroyed. Sorry I didn't notice this comment until now. The ideas behind my patch are the same, creating fewer tabs. However, we can't actually use the same tabs across modes because we must open new tabs in container mode.

Comment on attachment 8788099 [details] [diff] [review] Run multiple isolation tests in parallel. Review of attachment 8788099 [details] [diff] [review]: ----------------------------------------------------------------- > Since creating and retrieving blob urls doesn't affect each other. I let the four combinations reuse the opened tabs, instead of opening new tabs for each combinations. This reduce the running time from 25s to 17s on my laptop. I think this is enough to pass original timeout. Arthur and Baku, please take a look again. Thanks. Very cool that you found a way to get it to work! r+ after addressing my comments below. ::: browser/components/originattributes/test/browser/head.js @@ +283,5 @@ > firstFrameSetting = aURL.firstFrameSetting; > secondFrameSetting = aURL.secondFrameSetting; > } > > + if (!Array.isArray(aGetResultFunc)) { Maybe change aGetResultFunc to aGetResultFuncs for clarity? @@ +306,5 @@ > pageURL, > tabSettings[tabSettingB], > secondFrameSetting); > > + yield Promise.all(aGetResultFunc.map(getResultFunc => Task.spawn(function* () { I feel like the Task.spawn should be unnecessary as we are already inside a task, but perhaps with .map I am wrong... Also, is Promise.all needed here? Are we getting most of the performance improvement by sharing tabs, or is the parallelism helping as well? I worry we are more likely to run into bugs if the tests running in parallel, especially as they are running inside the same content page. But if parallelism does provide a performance improvement, then maybe it is worth the extra risk. If we drop the Promise.all, then it might be more readable to turn this into a simple loop for (let getResultFunc of aGetResultFunc) { let resultA = yield aGetResultFunc(tabInfoA.browser); let resultB = yield aGetResultFunc(tabInfoB.browser); etc... @@ +314,5 @@ > + > + // Compare results. > + let result = false; > + let shouldIsolate = (aMode !== TEST_MODE_NO_ISOLATION) && > + tabSettingA !== tabSettingB; I would suggest preserving the old indentation here. @@ +319,5 @@ > + if (aCompareResultFunc) { > + result = yield aCompareResultFunc(shouldIsolate, resultA, resultB); > + } else { > + result = shouldIsolate ? resultA !== resultB : > + resultA === resultB; Again I like the previous indentation.

Thanks, baku and Arthur. The performance improvement indeed mostly comes from tab sharing, so I remove the Promise.all.

I forgot to fix the indentation in the last patch.

Bug 1260931 landed, so I rebased this patch and turned on first party isolation mode.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d002007a8f1 Add blob url isolation tests, adapted from Tor Browser patch 15502. r=baku, r=arthuredelstein https://hg.mozilla.org/integration/mozilla-inbound/rev/4c154c6cbf4b Share tabs in isolation tests. r=baku, r=arthuredelstein

https://hg.mozilla.org/mozilla-central/rev/6d002007a8f1 https://hg.mozilla.org/mozilla-central/rev/4c154c6cbf4b