Closed Bug 1289319 Opened 7 years ago Closed 7 years ago

Add a test framework for the first party isolation tests.

Categories

(Core :: DOM: Security, defect, P1)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: timhuang, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor-testing][OA-testing][domsecurity-active])

Attachments

(1 file, 1 obsolete file)

Bug 1289319 - Add a test framework for the first party isolation tests.
58 bytes, text/x-review-board-request
baku
: review+
Details 
We need a test framework for the 1st party isolation tests, and followings are items that this bug should deal with:
  1. Create a new folder for all 1st party isolation tests.
  2. Add a new mochitest tag for these tests.
  3. Specify perf values, perfs of the 1st party isolation, for all tests in one place.
  4. A test framework that these tests can apply to the contextual identity as well.
Blocks: 1264577
Blocks: 1264560
Blocks: 1264562
Blocks: 1264567
Blocks: 1264572
Blocks: 1264573
Blocks: 1264574
Blocks: 1264581
Blocks: 1264593
Blocks: 1264595
Comment on attachment 8777230 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68280/diff/1-2/
Attachment #8777230 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/68848/#review65918

::: browser/components/firstpartyisolation/test/browser/head.js:124
(Diff revision 1)
> +   */
> +  add_task(aTask) {
> +    add_task(function* addTaskForIsolationTests() {
> +      // There should be one profile at least.
> +      if (this._profiles.length === 0) {
> +        ok(false, "It should be one profile at least.");

It should be -> There should be

::: browser/components/firstpartyisolation/test/browser/head.js:136
(Diff revision 1)
> +          info("Starting the test for the first party isolation.");
> +          this._isolationTestMode = TEST_MODE_FIRSTPARTY;
> +
> +          // Before run thes task, reset the preferences first.
> +          yield new Promise(resolve => {
> +            SpecialPowers.flushPrefEnv(resolve);

flushPrefEnv returns a Promise, so this should work:
```javascript
yield SpecialPowers.flushPrefEnv();
```

::: browser/components/firstpartyisolation/test/browser/head.js:140
(Diff revision 1)
> +          yield new Promise(resolve => {
> +            SpecialPowers.flushPrefEnv(resolve);
> +          });
> +
> +          // Enable the first party isolation.
> +          yield new Promise(resolve => {

pushPrefEnv also returns a promise.
```javascript
yield SpecialPowers.pushPrefEnv({"set": ...});
```

::: browser/components/firstpartyisolation/test/browser/head.js:157
(Diff revision 1)
> +          info("Starting the test for disabling isolation.");
> +          this._isolationTestMode = TEST_MODE_NO_ISOLATION;
> +
> +          // Before run thes task, reset the preferences first.
> +          yield new Promise(resolve => {
> +            SpecialPowers.flushPrefEnv(resolve);

the same

::: browser/components/firstpartyisolation/test/browser/head.js:172
(Diff revision 1)
> +          info("Starting the test for Containers.");
> +          this._isolationTestMode = TEST_MODE_CONTAINERS;
> +
> +          // Before run thes task, reset the preferences first.
> +          yield new Promise(resolve => {
> +            SpecialPowers.flushPrefEnv(resolve);

the same

::: browser/components/firstpartyisolation/test/browser/head.js:177
(Diff revision 1)
> +            SpecialPowers.flushPrefEnv(resolve);
> +          });
> +
> +          // Make sure userContext is enabled.
> +          yield new Promise(resolve => {
> +            SpecialPowers.pushPrefEnv({"set": [

the same
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68848/diff/1-2/
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68848/diff/2-3/
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68848/diff/3-4/
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68848/diff/4-5/
Attachment #8777245 - Flags: review?(tanvi)
The basic idea of the test framework is that you only have to write the test once, and the test framework will automatically change the preferences and run this test in different test settings, including the first party isolation, the no isolation, and the containers. 

The test framework will create the same tab in different ways according to the type of testing; given a URL, it will create a page with the first party domain, containing an iframe which loads the given URL for testing first party isolation, and will create a page loading the given URL with the user context id for testing containers. 

The ContentTask.spawn will also act differently here, the 'content' in the spawned task will be distinct for different testings. In the first party isolation, the 'content' will point to the window object of the iframe, and will remain the same as usual when testing containers. So the test can use the same code to test in different settings.
Attachment #8777245 - Flags: review?(arthuredelstein)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

https://reviewboard.mozilla.org/r/68848/#review67100

This seems to be a valid approach, although I would need to see it used with a real test to see how it works in practice. In the tests I wrote for caching by first party, I also ran tests on nested iframes (i.e., grandchild iframes), so it might be useful to consider how that might fit into this framework. Traditional <frame>s might also be something we should be testing.

There are a lot of uses of "this" and "bind" in these tests -- perhaps the use of closures instead will make the code simpler?

::: browser/components/firstpartyisolation/test/browser/head.js:78
(Diff revision 5)
> +function* openTabInFirstParty(aURL, aFirstPartyDomain) {
> +
> +  if (!aFirstPartyDomain.endsWith('/')) {
> +    aFirstPartyDomain += "/";
> +  }
> +

For simplicity I would be inclined never to include a "/" in a first party domain. Maybe insert it later when needed?

::: browser/components/firstpartyisolation/test/browser/head.js:185
(Diff revision 5)
> +          yield aTask();
> +        }.bind(this));
> +      }
> +    }.bind(this));
> +  },
> +

The three tasks added for TEST_MODE_FIRSTPARTY, TEST_MODE_NO_ISOLATION, and TEST_MODE_CONTAINERS have a lot of code in common. Maybe refactor them to reuse the same code, rather than repeat?
Attachment #8777245 - Flags: review?(arthuredelstein)
(In reply to Arthur Edelstein from comment #10)
> Comment on attachment 8777245 [details]
> Bug 1289319 - Add a test framework for the first party isolation tests.
> 
> https://reviewboard.mozilla.org/r/68848/#review67100
> 
> This seems to be a valid approach, although I would need to see it used with
> a real test to see how it works in practice. 

I have wrote a test case for the shared worker base on this framework at Bug 1264593.
And Jonathon also use this framework to write a test case of the local storage at Bug 1264567.

> In the tests I wrote for caching by first party, I also ran tests on nested iframes (i.e., grandchild
> iframes), so it might be useful to consider how that might fit into this
> framework. Traditional <frame>s might also be something we should be testing.

To accomplish this, I will add a new argument in the IsolationTestTools.addTab which is called frame setting. The frame setting allows you to specify the structure of how the page organizes frames. The setting is an array that first item represents the frame/iframe of the first layer, the second item for the second layer, and so on. For example, a frame setting [ iframe, frame, iframe ] will create a page with the following structure.

-|- Top level page
     |-- iframe 
          |-- frame // with the frameset 
               |-- iframe // loads with the target url

Through this frame setting, the test framework can create nested frames/iframes, and test traditional <frame>s.
Priority: -- → P1
Whiteboard: [tor-testing][OA-testing][domsecurity-active] → [tor-testing][OA-testing][domsecurity-active][ETA 9/12]
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

https://reviewboard.mozilla.org/r/68848/#review68916

::: browser/components/firstpartyisolation/test/browser/browser.ini:3
(Diff revision 6)
> +[DEFAULT]
> +skip-if = buildapp == "mulet"
> +tags = firstpartyisolation

What tags do we need here?  usercontextid, firstpartyisolation, and OriginAttributes?  Or just OriginAttributes?

::: browser/components/firstpartyisolation/test/browser/head.js:15
(Diff revision 6)
> +const TEST_MODE_FIRSTPARTY   = 0;
> +const TEST_MODE_NO_ISOLATION = 1;
> +const TEST_MODE_CONTAINERS   = 2;
> +
> +// The name of each mode.
> +const TEST_MODE_NAMES = [ "the first party isolation",

Remove the word "the" from these 3 lines.

::: browser/components/firstpartyisolation/test/browser/head.js:31
(Diff revision 6)
> +let gFirstPartyBasicPage = TEST_URL_PATH + "file_firstPartyBasic.html";
> +
> +/**
> + * Add a tab for the given url with the specific user context id.
> + *
> + * @param aURL The url of the page.

Put a dash or more spaces between parameter names and descriptions:

@param aURL - The url of the page
OR
@param aURL            The url of the page
@param aUserContextId  The user context...

OR 
@param aURL
   The url of the page

::: browser/components/firstpartyisolation/test/browser/head.js:58
(Diff revision 6)
> + * a frame setting to create nested frames. And this function will also modify
> + * the 'content' in the ContentTask to the target frame's window object.
> + *
> + * @param aURL The url of the iframe.
> + * @param aFirstPartyDomain The first party domain.
> + * @param aFrameSetting The setting of how to organize frames in the page.

Replace first sentence - This setting controls how frames are organized within the page.

::: browser/components/firstpartyisolation/test/browser/head.js:60
(Diff revision 6)
> + *
> + * @param aURL The url of the iframe.
> + * @param aFirstPartyDomain The first party domain.
> + * @param aFrameSetting The setting of how to organize frames in the page.
> + *                      The setting is an array of frame types, the first
> + *                      item indicates the frame type of the first layer of

...item indicated the frame time (iframe or frame) of the first layer of...

::: browser/components/firstpartyisolation/test/browser/head.js:62
(Diff revision 6)
> + * @param aFirstPartyDomain The first party domain.
> + * @param aFrameSetting The setting of how to organize frames in the page.
> + *                      The setting is an array of frame types, the first
> + *                      item indicates the frame type of the first layer of
> + *                      the frame structure, and the second item indicates
> + *                      the second layer, and so on. The page will be loaded

s/The page will be loaded/aURL will be loaded/

::: browser/components/firstpartyisolation/test/browser/head.js:90
(Diff revision 6)
> +  let browser = gBrowser.getBrowserForTab(tab);
> +  yield BrowserTestUtils.browserLoaded(browser);
> +
> +  let pageArgs = { url: aURL,
> +                   frames: aFrameSetting,
> +                   typeFrame: TEST_TYPE_FRAME,

Why do you need to pass in the two different frame types?

::: browser/components/firstpartyisolation/test/browser/head.js:99
(Diff revision 6)
> +  // Create the frame structure.
> +  yield ContentTask.spawn(browser, pageArgs, function* (arg) {
> +    let typeFrame = arg.typeFrame;
> +    let typeIFrame = arg.typeIFrame;
> +
> +    // Redefine the 'content' for allowing us to change its target, and making

Do you mean...
Redefine 'content' so that we can change its target and make sure ContentTask.spawn can directly work on the frame element?

I don't fully get this part.

::: browser/components/firstpartyisolation/test/browser/head.js:151
(Diff revision 6)
> +        // If it is the deepest layer, we load the target URL. Otherwise, we
> +        // load a basic page.
> +        if (numOfLayers === arg.frames.length) {
> +          frameElement.setAttribute("src", arg.url);
> +        } else {
> +          frameElement.setAttribute("src", arg.basicFrameSrc);

How does a frame in the middle embed the deepest most frame?  Since the basic page doesn't embed frames?  And the deepest most frame is within the basicFrameSrc page.

::: browser/components/firstpartyisolation/test/browser/head.js:174
(Diff revision 6)
> +                        [["privacy.firstparty.isolate", false]],
> +                        [["privacy.userContext.enabled", true]]
> +                      ],
> +
> +  /**
> +   * Adds isolation tests for the first party isolation, the no isolation

remove the prefixed "the"s

::: browser/components/firstpartyisolation/test/browser/head.js:218
(Diff revision 6)
> +      });
> +    }
> +  },
> +
> +  /**
> +   * Add a tab with the given profile, this will open different types of tabs

profile might be the wrong word here, since it can be confused with a browser profile.  Maybe the given setting or the given framework?  Or the given testing mode?

::: browser/components/firstpartyisolation/test/browser/head.js:225
(Diff revision 6)
> +   * target in different test mode; a profile indicates a first party domain
> +   * when testing the first party isolation, it is a user context id when
> +   * testing containers.
> +   *
> +   * @param aURL The url which is going to open.
> +   * @param aProfileId The profile id which will apply to this tab.

This will also have to be changed once we figure out the right word.

::: browser/components/firstpartyisolation/test/browser/head.js:308
(Diff revision 6)
> +  get isolationTestMode() {
> +    return this._isolationTestMode;
> +  },
> +
> +  /**
> +   * Return a boolean to indicate that should the test check the isolation is

to indicate whether the test should check if isolation is working

::: browser/components/moz.build:15
(Diff revision 6)
>      'customizableui',
>      'dirprovider',
>      'downloads',
>      'extensions',
>      'feeds',
> +    'firstpartyisolation',

Instead of putting this in browser/components/firstpartyisolation, I think all these files shoudl be in browser/components/OriginAttributes.  Then future OriginAttribute consumers can add their tests here and add to your framework.
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Adding baku as a reviewer.
Attachment #8777245 - Flags: review?(tanvi) → review?(amarchesini)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Actually removing baku and we can add him on a newer version.
Attachment #8777245 - Flags: review?(amarchesini)
(In reply to Tanvi Vyas [:tanvi] from comment #13)
> Comment on attachment 8777245 [details]
> Bug 1289319 - Add a test framework for the first party isolation tests.
> 
> https://reviewboard.mozilla.org/r/68848/#review68916
> 
> ::: browser/components/firstpartyisolation/test/browser/browser.ini:3
> (Diff revision 6)
> > +[DEFAULT]
> > +skip-if = buildapp == "mulet"
> > +tags = firstpartyisolation
> 
> What tags do we need here? usercontextid, firstpartyisolation, and
> OriginAttributes? Or just OriginAttributes?
> 

I think we should use 'tags = usercontextid, firstpartyisolation, originattributes' here. Because there might be other test cases for userContextId and first party isolation that are not related to originAttributes. So they could still run tests at here by using these tags. 

> ::: browser/components/firstpartyisolation/test/browser/head.js:90
> (Diff revision 6)
> > + let browser = gBrowser.getBrowserForTab(tab);
> > + yield BrowserTestUtils.browserLoaded(browser);
> > +
> > + let pageArgs = { url: aURL,
> > + frames: aFrameSetting,
> > + typeFrame: TEST_TYPE_FRAME,
> 
> Why do you need to pass in the two different frame types?
> 

Because the creating of the frame structure will be run on the content side, and the content side is unable to know these types unless we pass frame types into content.

> ::: browser/components/firstpartyisolation/test/browser/head.js:99
> (Diff revision 6)
> > + // Create the frame structure.
> > + yield ContentTask.spawn(browser, pageArgs, function* (arg) {
> > + let typeFrame = arg.typeFrame;
> > + let typeIFrame = arg.typeIFrame;
> > +
> > + // Redefine the 'content' for allowing us to change its target, and making
> 
> Do you mean...
> Redefine 'content' so that we can change its target and make sure
> ContentTask.spawn can directly work on the frame element?
> 
> I don't fully get this part.
> 

Yes, you are right. Because the 'content' is pointing to the window object of the top-level page at the first place. So we redefine the 'content' here allowing us changing its target to the window object of the frame element.

> ::: browser/components/firstpartyisolation/test/browser/head.js:151
> (Diff revision 6)
> > + // If it is the deepest layer, we load the target URL. Otherwise, we
> > + // load a basic page.
> > + if (numOfLayers === arg.frames.length) {
> > + frameElement.setAttribute("src", arg.url);
> > + } else {
> > + frameElement.setAttribute("src", arg.basicFrameSrc);
> 
> How does a frame in the middle embed the deepest most frame? Since the
> basic page doesn't embed frames? And the deepest most frame is within the
> basicFrameSrc page.
> 

I use document.createElement() to create the frame/iframe which will load the deepest frame in the middle layer.

> ::: browser/components/firstpartyisolation/test/browser/head.js:218
> (Diff revision 6)
> > + });
> > + }
> > + },
> > +
> > + /**
> > + * Add a tab with the given profile, this will open different types of tabs
> 
> profile might be the wrong word here, since it can be confused with a
> browser profile. Maybe the given setting or the given framework? Or the
> given testing mode?
>

I think 'given tab setting' is good.
 
> ::: browser/components/firstpartyisolation/test/browser/head.js:225
> (Diff revision 6)
> > + * target in different test mode; a profile indicates a first party domain
> > + * when testing the first party isolation, it is a user context id when
> > + * testing containers.
> > + *
> > + * @param aURL The url which is going to open.
> > + * @param aProfileId The profile id which will apply to this tab.
> 
> This will also have to be changed once we figure out the right word.
> 

It could be 'aTabSettingId' or 'aSettingId'.

> ::: browser/components/firstpartyisolation/test/browser/head.js:308
> (Diff revision 6)
> > + get isolationTestMode() {
> > + return this._isolationTestMode;
> > + },
> > +
> > + /**
> > + * Return a boolean to indicate that should the test check the isolation is
> 
> to indicate whether the test should check if isolation is working
> 
> ::: browser/components/moz.build:15
> (Diff revision 6)
> > 'customizableui',
> > 'dirprovider',
> > 'downloads',
> > 'extensions',
> > 'feeds',
> > + 'firstpartyisolation',
> 
> Instead of putting this in browser/components/firstpartyisolation, I think
> all these files shoudl be in browser/components/OriginAttributes. Then
> future OriginAttribute consumers can add their tests here and add to your
> framework.

This is a good idea, I will change the folder's name.
Attachment #8777245 - Flags: review?(arthuredelstein)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

https://reviewboard.mozilla.org/r/68848/#review70198

Why don't you use browser/components/contextualidentity?

::: browser/components/originattributes/test/browser/browser.ini:2
(Diff revision 7)
> +[DEFAULT]
> +skip-if = buildapp == "mulet"

I don't think we have mulet anymore.
Attachment #8777245 - Flags: review?(amarchesini) → review-
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

https://reviewboard.mozilla.org/r/68848/#review70212

I&#39;m not totally against having a originAttributes directory. But it&#39;s confusing having contextualidentity and originattributes. Maybe we can merge the 2 folder?
For the rest. r+.
Attachment #8777245 - Flags: review- → review+
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Sorry for the delay in reviewing. I like the overall idea of this framework, but I find the code quite difficult to follow. The framework seems to have various stateful member variables and several API function calls. I think the internal workings and the external API of this test framework could be simplified considerably. Why not expose a single function that does all the work and doesn't maintain any state? This will make the tests easier to debug and simpler to follow.

For example, the entire API could look like:
IsolationTestTools.runTests(generateExampleContent);

The generateExampleContent(contentWindow, profile) function would generate page content, and return results that could be compared by the framework to check if isolation is working.

Here's some pseudocode to show how I imagine the framework could operate internally:

  for (prefs, profileA, profileB) {
    yield pushPrefs(pref);
    let useFrames = pref === [["privacy.firstparty.isolate"]];
    let tabA = yield createExampleTab(profileA, useFrames);
    let resultA = yield generateExampleContent(tabA.contentWindow);
    let tabB = yield createExampleTab(profileB, useFrames);
    let resultB = yield generateExampleContent(tabB.contentWindowB);
    yield closeTabs(tabA, tabB);
    let shouldIsolate = getExpectedIsolationState(prefs, profileA, profileB);
    ok(shouldIsolate ? resultA !== resultB : resultA === resultB, msg);
  }

Internally, the framework could generate standard combinations of profiles and prefs, just as you have already provided standard combinations of prefs. The API caller shouldn't need to specify these.

Also, I don't see any need to have options to skip tests -- we need them all to pass, right? So removing that stuff can simplify things further.

So my suggestion is to refactor the framework to make using it as simple as possible.
Attachment #8777245 - Flags: review?(arthuredelstein) → review-
(In reply to Andrea Marchesini [:baku] from comment #19)
> Comment on attachment 8777245 [details]
> Bug 1289319 - Add a test framework for the first party isolation tests.
> 
> https://reviewboard.mozilla.org/r/68848/#review70212
> 
> I&#39;m not totally against having a originAttributes directory. But
> it&#39;s confusing having contextualidentity and originattributes. Maybe we
> can merge the 2 folder?
> For the rest. r+.

I think it would be better if we had two folders for contextualidenity and originattributes respectively. Because there might be some UI tests that don't relate to the originAttributes in the Containers, for example, the browser_middleClick.js. And I imagine there will be more such test cases when we involve more UI for Containers. Besides, just as Tanvi said, there could be more OriginAttribute consumers in the future, and they might have nothing to do with the containers. So, we still need a contextualidentity folder to put Containers stuff.
(In reply to Arthur Edelstein from comment #20)
> Comment on attachment 8777245 [details]
> Bug 1289319 - Add a test framework for the first party isolation tests.
> 

I think it's a good way to simplify how people using this framework. And I have some ideas about this.

> Sorry for the delay in reviewing. I like the overall idea of this framework,
> but I find the code quite difficult to follow. The framework seems to have
> various stateful member variables and several API function calls. I think
> the internal workings and the external API of this test framework could be
> simplified considerably. Why not expose a single function that does all the
> work and doesn't maintain any state? This will make the tests easier to
> debug and simpler to follow.
> 
> For example, the entire API could look like:
> IsolationTestTools.runTests(generateExampleContent);
> 
> The generateExampleContent(contentWindow, profile) function would generate
> page content, and return results that could be compared by the framework to
> check if isolation is working.
> 

I think it would look like:
IsolationTestTools.runTests(aURL, generateAndReturnResultFunc, (optional) resultCompareFunc);

The aURL is the page URL which will be loaded in frames.

The generateAndReturnResultFunc(browser, tabSettingId) would modify the page content through ContentTask.spawn() by using the 'browser' to modify the page content and fetch the result from the page. And we change the name 'profile' to 'tabSetting' for avoiding confusing with firefox profiles.

The resultCompareFunc(isIsolated, resultA, resultB) is an optional function which returns a boolean to indicate whether the isolation is working. If it is not given, the framework will compare the results by itself. The main purpose of this function is to provide certain flexibility. It would be useful when we need additional information other than the results from the page content to check isolation. For example, when we testing the cache isolation, we need internal APIs to check the cache, that is unlikely fetched from the page content.


> Here's some pseudocode to show how I imagine the framework could operate
> internally:
> 
>   for (prefs, profileA, profileB) {
>     yield pushPrefs(pref);
>     let useFrames = pref === [["privacy.firstparty.isolate"]];
>     let tabA = yield createExampleTab(profileA, useFrames);
>     let resultA = yield generateExampleContent(tabA.contentWindow);
>     let tabB = yield createExampleTab(profileB, useFrames);
>     let resultB = yield generateExampleContent(tabB.contentWindowB);
>     yield closeTabs(tabA, tabB);
>     let shouldIsolate = getExpectedIsolationState(prefs, profileA, profileB);
>     ok(shouldIsolate ? resultA !== resultB : resultA === resultB, msg);
>   }
> 
> Internally, the framework could generate standard combinations of profiles
> and prefs, just as you have already provided standard combinations of prefs.
> The API caller shouldn't need to specify these.
> 

Yes, the framework could take care of this part.

> Also, I don't see any need to have options to skip tests -- we need them all
> to pass, right? So removing that stuff can simplify things further.
> 

The original thought of these functions is for debugging that you can disable other test modes if you want to focus one of them. Maybe we could still remain this feature for skipping tests but remove these functions from the framework.

How do you think, Arthur?

> So my suggestion is to refactor the framework to make using it as simple as
> possible.
Flags: needinfo?(arthuredelstein)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

Hi, Arthur.

I had updated the test framework, and you can find an example based on the IsolationTestTools.runTests() at Bug 1264593, thanks.
Attachment #8777245 - Flags: review?(arthuredelstein)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

baku, can you also review the new api?
Attachment #8777245 - Flags: review+ → review?(amarchesini)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

https://reviewboard.mozilla.org/r/68848/#review71112

This external API looks much better to me now, thanks! I think the internal structure could still be simplified and clarified further, however, mainly by removing member variables and getters (unnecessary mutable state) and instead passing things like testMode and tabSettings as function arguments (see my suggested changes below). (Essentially, I see "self" and "this" as code smells.) I would suggest marking all functions that are not part of the external API as private in some way (such as with a leading underscore). Also, I think it would be nice for readability if you re-order function definitions in the file such that functions defined later only call functions defined earlier.

> I had updated the test framework, and you can find an example based on the
> IsolationTestTools.runTests() at Bug 1264593, thanks.

I like how this SharedWorker example looks now -- nice and simple! :)

Have you tested whether the SharedWorker tests in Bug 1264593 pass when the patch from Bug 1294336 is applied, and that some fail when the patch is not applied? I think that would be a good way to make sure that everything in this framework is working correctly before landing it.

r+ pending cleanup and confirmation with the SharedWorker example. Great work -- thank you!

::: browser/components/originattributes/test/browser/head.js:168
(Diff revision 8)
> +
> +  return {tab, browser};
> +}
> +
> +this.IsolationTestTools = {
> +  _isolationTestMode: TEST_MODE_FIRSTPARTY,

Remove \_isolationTestMode as a member variable. Instead, pass a test mode as a function argument in add_task.

::: browser/components/originattributes/test/browser/head.js:169
(Diff revision 8)
> +  return {tab, browser};
> +}
> +
> +this.IsolationTestTools = {
> +  _isolationTestMode: TEST_MODE_FIRSTPARTY,
> +  _tabSettings:       [],

Remove \_tabSettings as a member variable. Just pass testTabSettings[i] as a function argument when needed.

::: browser/components/originattributes/test/browser/head.js:170
(Diff revision 8)
> +}
> +
> +this.IsolationTestTools = {
> +  _isolationTestMode: TEST_MODE_FIRSTPARTY,
> +  _tabSettings:       [],
> +  _skipTestFlags:     [false, false, false],

Remove \_skipTestFlags and simply let people comment out \_testPrefs or \_testTabSettings when they are debugging.

::: browser/components/originattributes/test/browser/head.js:201
(Diff revision 8)
> +   *    check results. This function will be provided a boolean to indicate
> +   *    the isolation is no or off and two results. This function should return
> +   *    a boolean to tell that whether isolation is working. If this function
> +   *    is not given, the framework will take case checking by itself.
> +   */
> +  runTests(aURL, aGetResultFunc, aCompareResultFunc) {

This API looks great. Nicely documented as well.

::: browser/components/originattributes/test/browser/head.js:215
(Diff revision 8)
> +      firstFrameSetting = aURL.firstFrameSetting;
> +      secondFrameSetting = aURL.secondFrameSetting;
> +    }
> +
> +    // Register tab settings.
> +    this.registerTabSetting(this._testTabSettings[0].firstPartyDomain,

No need to register tab settings here. Just pass the tab settings themselves, below.

::: browser/components/originattributes/test/browser/head.js:221
(Diff revision 8)
> +                            this._testTabSettings[0].userContextId);
> +    this.registerTabSetting(this._testTabSettings[1].firstPartyDomain,
> +                            this._testTabSettings[1].userContextId);
> +
> +    let self = this;
> +    this.add_task(function* () {

This function* () should accept aTestMode as an argument.

::: browser/components/originattributes/test/browser/head.js:227
(Diff revision 8)
> +      let tabSettingA = 0;
> +
> +      for (let tabSettingB of [0, 1]) {
> +        // Create Tabs.
> +        let tabInfoA = yield self.addTab(pageURL, tabSettingA, firstFrameSetting);
> +        let tabInfoB = yield self.addTab(pageURL, tabSettingB, secondFrameSetting);

Pass \_testTabSettings\[0\] and \_testTabSettings\[1\] here instead of tabSettingA, tabSettingB. Also, pass aTestMode here.

::: browser/components/originattributes/test/browser/head.js:239
(Diff revision 8)
> +        yield BrowserTestUtils.removeTab(tabInfoA.tab);
> +        yield BrowserTestUtils.removeTab(tabInfoB.tab);
> +
> +        // Compare results.
> +        let result = false;
> +        let shouldIsolate = self.shouldCheckIsolation &&

Rather than use a shouldCheckIsolation getter, just check aTestMode === TEST_MODE_NO_ISOLATION directly here.

::: browser/components/originattributes/test/browser/head.js:248
(Diff revision 8)
> +        } else {
> +          result = shouldIsolate ? resultA !== resultB :
> +                                   resultA === resultB;
> +        }
> +
> +        let msg = `Testing ${TEST_MODE_NAMES[self.isolationTestMode]} for ` +

Use [aTestMode]

::: browser/components/originattributes/test/browser/head.js:298
(Diff revision 8)
> +        yield SpecialPowers.flushPrefEnv();
> +
> +        // Make sure preferences are set properly.
> +        yield SpecialPowers.pushPrefEnv({"set": self._testPrefs[aMode]});
> +
> +        yield aTask();

Use `yield aTask(aMode);`

::: browser/components/originattributes/test/browser/head.js:324
(Diff revision 8)
> +   *    will be loaded at the deepest layer. This is optional.
> +   *
> +   * @return tab     - The tab object of this tab.
> +   *         browser - The browser object of this tab.
> +   */
> +  addTab(aURL, aTabSettingId, aFrameSetting) {

To simplify here, pass aTabSetting object, rather than aTabSettingId. Also, you pass aTestMode.

::: browser/components/originattributes/test/browser/head.js:325
(Diff revision 8)
> +   *
> +   * @return tab     - The tab object of this tab.
> +   *         browser - The browser object of this tab.
> +   */
> +  addTab(aURL, aTabSettingId, aFrameSetting) {
> +    let testMode = this._isolationTestMode;

Remove this line in favor of aTestMode argument.

::: browser/components/originattributes/test/browser/head.js:347
(Diff revision 8)
> +   *    The user context id of this tab setting.
> +   *
> +   * return id - A tab setting id which starts from 0 and increases by one as
> +   * new setting added.
> +   */
> +  registerTabSetting(aFirstPartyDomain, aUserContextId = 0) {

Remove this function (unnecessary).

::: browser/components/originattributes/test/browser/head.js:365
(Diff revision 8)
> +   *    The id of the tab setting.
> +   *
> +   * return firstPartyDomain - The first party domain of this tab setting.
> +   *        userContextId    - The user context id of this tab setting.
> +   */
> +  getTabSetting(aTabSettingId) {

Also remove.

::: browser/components/originattributes/test/browser/head.js:374
(Diff revision 8)
> +  /**
> +   * Get current test mode.
> +   *
> +   * return mode - The current test mode.
> +   */
> +  get isolationTestMode() {

Remove along with _isolationTestMode.

::: browser/components/originattributes/test/browser/head.js:382
(Diff revision 8)
> +
> +  /**
> +   * Return a boolean to to indicate whether the test should check if isolation
> +   * is working.
> +   */
> +  get shouldCheckIsolation() {

Eliminate this getter; unnecessary.
Attachment #8777245 - Flags: review?(arthuredelstein) → review+
Flags: needinfo?(arthuredelstein)
Comment on attachment 8777245 [details]
Bug 1289319 - Add a test framework for the first party isolation tests.

https://reviewboard.mozilla.org/r/68848/#review71318

::: browser/components/originattributes/test/browser/head.js:168
(Diff revision 8)
> +
> +  return {tab, browser};
> +}
> +
> +this.IsolationTestTools = {
> +  _isolationTestMode: TEST_MODE_FIRSTPARTY,

This should be: _currentTestMode but it will be removed if you apply the comments of Arthur.

::: browser/components/originattributes/test/browser/head.js:276
(Diff revision 8)
> +        return;
> +      }
> +
> +      let testModes = [ TEST_MODE_FIRSTPARTY,
> +                        TEST_MODE_NO_ISOLATION,
> +                        TEST_MODE_CONTAINERS ];

The configuration of these modes are here and in _skipTestFlags and in _testPrefs.

What about if you merge them in:

let tests = [
 { skipFlag: false,
   prefs: [["privacy.firstparty.isolate", true]] },
 { skipFlag: false,
   ...

and remove _skipTestFlags and _testPrefs.
Attachment #8777245 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #27)
> Comment on attachment 8777245 [details]
> Bug 1289319 - Add a test framework for the first party isolation tests.
> 
> https://reviewboard.mozilla.org/r/68848/#review71318

> The configuration of these modes are here and in _skipTestFlags and in
> _testPrefs.
> 
> What about if you merge them in:
> 
> let tests = [
>  { skipFlag: false,
>    prefs: [["privacy.firstparty.isolate", true]] },
>  { skipFlag: false,
>    ...
> 
> and remove _skipTestFlags and _testPrefs.

I think I will merge testMode, _skipTestFlags and _testPrefs here. Something like ...

let tests = [
  { mode: TEST_MODE_FIRSTPARTY,
    skipFlag: false,
    prefs: [["privacy.firstparty.isolate", true]] },
  { mode: TEST_MODE_NO_ISOLATION,
    skipFlag: false,
    prefs: [["privacy.firstparty.isolate", false]] },
    ...

And the addTaskForMode() will be changed to addTaskForMode(aMode, aPref, aSkip, aTask). Arguments of this function correspond to fields in the 'tests'.
I had tested the Bug 1264593 with the Bug 1260931 and Bug 1294336. The result shows that the test framework is working as we expect.
Try looks good.
Keywords: checkin-needed
For future reference, when you've got a patch that's only adding new browser-chrome tests, it's a waste of limited Try resources running all the other test suites as well. Please be more conscientious of that in the future.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0f17004b8c21
Add a test framework for the first party isolation tests. r=arthuredelstein+474980,baku
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> For future reference, when you've got a patch that's only adding new
> browser-chrome tests, it's a waste of limited Try resources running all the
> other test suites as well. Please be more conscientious of that in the
> future.

Thanks, Ryan. I will be more conscientious in the future.
https://hg.mozilla.org/mozilla-central/rev/0f17004b8c21
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox51: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [tor-testing][OA-testing][domsecurity-active][ETA 9/12] → [tor-testing][OA-testing][domsecurity-active]
Target Milestone: --- → mozilla51
Blocks: meta_tor
This bug added a moz.build pointer to a non-existent Firefox::OriginAttributes bug component. Were their plans to add one? Please either get it created or change the component to one more suitable.
Flags: needinfo?(tihuang)
AFAIK, we havn't discussed about having this bug component. So, I think we should change the component to Core::DOM:Security. I will fire a bug for doing this, thanks.
Flags: needinfo?(tihuang)
See Also: → 1368423
You need to log in before you can comment on or make changes to this bug.