Regression tests for blob URL isolation (Tor 15502)

RESOLVED FIXED in Firefox 51

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: timhuang, Assigned: jhao)

Tracking

(Blocks 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1][ETA 11/7])

Attachments

(3 attachments, 9 obsolete attachments)

14.96 KB, patch
Details | Diff | Splinter Review
6.08 KB, patch
jhao
: review+
Details | Diff | Splinter Review
5.85 KB, patch
jhao
: review+
Details | Diff | Splinter Review
No description provided.
We should add a test case for tor broswer for Regression tests for blob URL isolation. https://torpat.ch/15502
Whiteboard: [tor], [OA] → [tor], [OA][domsecurity-backlog]
Summary: Regression tests for blob URL isolation (Tor Bug#15502) → Regression tests for blob URL isolation (Tor 15502)
Whiteboard: [tor], [OA][domsecurity-backlog] → [tor][OA-testing][domsecurity-backlog]
Whiteboard: [tor][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog]
Priority: -- → P1
Depends on: 1289319
Priority: P1 → P3
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog1]
Priority: P3 → P1
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1] → [tor-testing][OA-testing][domsecurity-backlog1][ETA 11/7]
Priority: P1 → P2
Assignee: nobody → jhao
Status: NEW → ASSIGNED
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.
Attachment #8785232 - Attachment is obsolete: true
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.
Attachment #8786011 - Attachment is obsolete: true
Attachment #8786594 - Flags: review?(arthuredelstein)
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.
Attachment #8786594 - Flags: review?(arthuredelstein) → review+
Thanks, Arthur.

Hi Baku, could you also review this patch?
Attachment #8787125 - Flags: review?(amarchesini)
Attachment #8786594 - Attachment is obsolete: true
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})
Attachment #8787125 - Flags: review?(amarchesini) → review+
Attachment #8787125 - Attachment is obsolete: true
Keywords: checkin-needed
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
Keywords: checkin-needed
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
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.
Flags: needinfo?(jhao) → needinfo?(arthuredelstein)
(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.
Flags: needinfo?(arthuredelstein)
(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.
Attachment #8788029 - Flags: review+
Attachment #8787535 - Attachment is obsolete: true
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.
Attachment #8788099 - Flags: review?(arthuredelstein)
Attachment #8788099 - Flags: review?(amarchesini)
Attachment #8788032 - Attachment is obsolete: true
Attachment #8788099 - Flags: review?(amarchesini) → review+
(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.
Attachment #8788099 - Flags: review?(arthuredelstein) → review+
Thanks, baku and Arthur.  The performance improvement indeed mostly comes from tab sharing, so I remove the Promise.all.
Attachment #8788341 - Flags: review+
Attachment #8788099 - Attachment is obsolete: true
I forgot to fix the indentation in the last patch.
Attachment #8788346 - Flags: review+
Attachment #8788341 - Attachment is obsolete: true
Bug 1260931 landed, so I rebased this patch and turned on first party isolation mode.
Attachment #8788756 - Flags: review+
Attachment #8788029 - Attachment is obsolete: true
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d002007a8f1
https://hg.mozilla.org/mozilla-central/rev/4c154c6cbf4b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.