Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13749 closed task (fixed)

Write tests for privacy.thirdparty.isolate

Reported by: mikeperry Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201501R, tbb-4.5-alpha-3, MikePerry201501R
Cc: gk, boklm, mcs, michael Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should write tests for testing DOM storage, image cache, and content cache isolation. I think that mochitests are the best way to go, as they allow you to fetch things from different domains:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

I think all of the stuff behind network.thirdparty.isolate should be our main priority of things to merge with Mozilla. Historically, these components also have a high rate of breaking between releases, so we should get this coverage as soon as possible.

Child Tickets

Attachments (2)

0001-Bug-13749.1-regression-tests-for-first-party-isolati.patch (6.6 KB) - added by arthuredelstein 5 years ago.
0001-Bug-13749.2-Regression-tests-for-first-party-isolati.patch (35.4 KB) - added by arthuredelstein 5 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by gk

Cc: gk boklm added

Hm... I actually think we should try to get that into an xpcshell test as these are more lightweight and Mozilla recommends using mochitests only if we can't use xpcshell (see: http://ehsanakhgari.org/wp-content/uploads/talks/test-mozilla/ slide 15). And having different domains should be not a problem either as one can add identities to the HTTP server (see e.g.: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/head_psm.js#434). If that means "You get your xpcshell test only if you write it!" then be it so. :) This might even help us directly as our xpcshell testing is not as broken as our mochitest testing IIRC (but I might be wrong here).

Last edited 5 years ago by gk (previous) (diff)

comment:2 Changed 5 years ago by gk

I had #13472 in mind when I wrote my previous comment. Thus, to clarify: testing whether the cache key is changed at all (contains the domain) should be in an xpcshell test if we write a separate test for it. I need a bit more time to think about the other things we want to test here. I don't want Mozilla complaining about choosing the wrong test suite when we are adding the tests to the patches we want to get merged.

comment:3 Changed 5 years ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:4 Changed 5 years ago by mikeperry

gk - that's true, but I wanted to involve as many aspects of the page rendering as possible. The new patch is based on our getFirstPartyURI() API. Part of testing this is making sure that API also is behaving sanely for iframes, script src, link rel, etc etc.

I also just realized that we should probably also test Etag isolation as part of this. In previous versions of this cache isolation feature, it was covered. That may no longer be the case (or it may not be the case when the transition to Cache2 is complete).

comment:5 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201412 added; TorBrowserTeam201411 removed

comment:6 Changed 5 years ago by arthuredelstein

First part posted for review (image and content patches still to come).

comment:7 Changed 5 years ago by mcs

Cc: boklm. mcs added; boklm removed

comment:8 Changed 5 years ago by mcs

Cc: boklm added; boklm. removed

comment:9 Changed 5 years ago by arthuredelstein

Status: assignedneeds_review

comment:10 Changed 5 years ago by arthuredelstein

Summary: Write tests for network.thirdparty.isolateWrite tests for privacy.thirdparty.isolate

comment:11 in reply to:  10 Changed 5 years ago by arthuredelstein

Please disregard the second patch here -- I'm developing a more comprehensive test that will supersede it. The first patch is ready for review, however.

comment:12 Changed 5 years ago by michael

Cc: michael added

comment:13 Changed 5 years ago by arthuredelstein

I've replaced the second patch with a new versions. Now both patches are ready for review.

comment:14 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201501 added; TorBrowserTeam201412 removed

comment:15 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201501R added; TorBrowserTeam201501 removed

comment:16 Changed 5 years ago by arthuredelstein

To confirm that the second patch reports what we think it should, I first reverted the cache patch

> git revert 8425b9a358211777543303d2194b378b2765f144
[13749 e29d844] Revert "Bug 13742: Isolate cache to URL bar domain."
 1 file changed, 2 insertions(+), 16 deletions(-)

The result was all tests when the isolation is activated (including image caching) fail:

 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for iframe.html: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for link.css: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for script.js: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for img.png: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for object.png: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for embed.png: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for xhr.html: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for audio.ogg: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for video.ogv: 4
 0:29.04 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for track.vtt: 4

then I brought back that patch and instead reverted the image cache isolation patch:

> git revert 114cd22282f8b3cd6e6a5c29de8a8c396a79acc0
[13749 0236e36] Revert "Bug #6539: Isolate the Image Cache per url bar domain."
 12 files changed, 156 insertions(+), 274 deletions(-)

and the result was that image caching tests fail:

 0:32.53 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for img.png: 4
 0:32.53 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for object.png: 4
 0:32.53 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for embed.png: 4
 0:32.53 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for img.png: 1
 0:32.53 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for object.png: 1
 0:32.53 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/netwerk/test/browser/browser_cacheFirstParty.js | Cache entries expected for embed.png: 1

So I feel pretty comfortable that these regression tests are testing what they are supposed to be testing.

comment:17 Changed 5 years ago by mikeperry

Keywords: tbb-4.5-alpha-3 added

comment:18 Changed 5 years ago by mikeperry

Keywords: MikePerry201501R added

comment:19 Changed 5 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, that sounds good. The test is a little hard to follow, but seems ok. I have merged this into the 4.5-alpha branch in tor-browser.git.

comment:20 in reply to:  19 Changed 5 years ago by arthuredelstein

Replying to mikeperry:

Ok, that sounds good. The test is a little hard to follow, but seems ok.

Anything specific that I should clarify?

Note: See TracTickets for help on using tickets.