Implement private browsing for mozbrowser #11544

jdm · 2016-06-01T16:38:31Z

Support the mozprivatebrowsing attribute on mozbrowser iframes. This separates the non-private and private sessions in terms of cookies, HSTS lists, cached HTTP credentials, HTTP connection pools, and web storage. The private session is shared between all private mozbrowsers, and lasts until shutdown.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

This change is

highfive · 2016-06-01T16:38:34Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs, components/constellation/pipeline.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/net/resource_thread.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/websocket.rs, components/script/dom/webidls/HTMLIFrameElement.webidl

asajeffrey · 2016-06-01T17:44:14Z

Mostly this looks good, but there is one big change I think is worth making.

This PR adds the pipeline id to each of the resource messages, but not the privacy level of the pipeline. Since the resource thread doesn't know the privacy level, it has to send a message to the constellation asking it. This adds quite a bit of complexity to the code, and involves the constellation in every resource access. Life would be a lot simpler if the resource messages included the privacy level of the pipeline, not just the pipeline id.

Previously, highfive wrote…

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs, components/constellation/pipeline.rs

@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/net/resource_thread.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/websocket.rs, components/script/dom/webidls/HTMLIFrameElement.webidl

Reviewed 6 of 9 files at r1, 3 of 4 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

components/constellation/constellation.rs, line 105 [r3] (raw file):

    /// Receives ConstellationMsg
    pub resource_msg_receiver: Receiver<FromResourceMsg>,

This channel wouldn't be needed if the resource messages included the privacy level.

components/constellation/constellation.rs, line 340 [r3] (raw file):

                panic_sender: ipc_panic_sender,
                compositor_receiver: compositor_receiver,
                resource_msg_receiver: resource_receiver,

This channel wouldn't be needed if the resource messages included the privacy level.

components/constellation/constellation.rs, line 420 [r3] (raw file):

                    script_channel: Option<IpcSender<ConstellationControlMsg>>,
                    load_data: LoadData,
                    is_private: bool) {

Better to use an enum for this?

components/constellation/constellation.rs, line 503 [r3] (raw file):

            Layout(FromLayoutMsg),
            Panic(PanicMsg),
            Resource(FromResourceMsg),

This case wouldn't be needed if the resource messages included the privacy level.

components/constellation/constellation.rs, line 533 [r3] (raw file):

                    Request::Panic(msg.expect("Unexpected panic channel panic in constellation")),
                msg = receiver_from_constellation_msg.recv() =>
                    Request::Resource(msg.expect("Unexpected resource failure in constellation"))

This branch wouldn't be needed if the resource messages included the privacy level.

components/constellation/constellation.rs, line 560 [r3] (raw file):

    }

    fn handle_request_from_resource(&self, message: FromResourceMsg) {

This handler wouldn't be needed if the resource messages included the privacy level.

components/net/resource_thread.rs, line 412 [r1] (raw file):

            read_json_from_file(&mut cookie_jar, config_dir, "cookie_jar.json");
        }
        let resource_group = ResourceGroup {

Add ResourceGroup::new?

components/net/resource_thread.rs, line 395 [r3] (raw file):

    cancel_load_map: HashMap<ResourceId, Sender<()>>,
    next_resource_id: ResourceId,
    constellation_msg_chan: Option<IpcSender<ConstellationMsg>>,

This channel wouldn't be needed if the resource messages included the privacy level.

components/net/resource_thread.rs, line 520 [r3] (raw file):

    }

    fn get_resource_group(&self, pipeline_id: Option<PipelineId>) -> ResourceGroup {

This is sending a message to the constellation every time a resource is accessed. Wouldn't it be easier to make the parameter to this function a resource group name (or just is_private: bool if you prefer). If the resource messages included the pipeline's privacy level, this means the constellation wouldn't be involved in checking privacy levels.

components/net_traits/lib.rs, line 333 [r3] (raw file):

    Load(LoadData, LoadConsumer, Option<IpcSender<ResourceId>>),
    /// Try to make a websocket connection to a URL.
    WebsocketConnect(PipelineId, WebSocketCommunicate, WebSocketConnectData),

Life would be a lot easier if you included the privacy level of the pipeline in this message.

components/net_traits/lib.rs, line 343 [r3] (raw file):

    Synchronize(IpcSender<()>),
    /// Send the channel for checking a private pipeline
    SendConstellationMsgChannel(IpcSender<ConstellationMsg>),

This message wouldn't be needed if the resource messages included the privacy level.

components/script/dom/htmliframeelement.rs, line 495 [r3] (raw file):

    // check-tidy: no specs after this line
    fn SetMozprivatebrowsing(&self, value: bool) {
        let element = self.upcast::<Element>();

Check if mozbrowser is enabled?

components/script/dom/htmliframeelement.rs, line 499 [r3] (raw file):

    }

    fn Mozprivatebrowsing(&self) -> bool {

Check if mozbrowser is enabled?

Comments from Reviewable

bors-servo · 2016-06-04T04:13:16Z

☔ The latest upstream changes (presumably #11585) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2016-06-04T15:02:57Z

IRC chat: http://logs.glob.uno/?c=mozilla%23servo&s=2+Jun+2016&e=2+Jun+2016&h=private#c446852

asajeffrey · 2016-06-10T17:02:59Z

OK, looks much nicer!

Previously, asajeffrey (Alan Jeffrey) wrote…

IRC chat: http://logs.glob.uno/?c=mozilla%23servo&s=2+Jun+2016&e=2+Jun+2016&h=private#c446852

Reviewed 8 of 9 files at r4, 4 of 4 files at r5, 3 of 3 files at r6, 9 of 9 files at r7.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 424 [r6] (raw file):

                    load_data: LoadData,
                    is_private: bool) {
        if self.shutting_down { return; }

This change doesn't appear to have much to do with private browsing, care to comment?

components/net/resource_thread.rs, line 209 [r7] (raw file):

    let mut hsts_list = HstsList::from_servo_preload();
    let mut auth_cache = AuthCache::new();
    let mut cookie_jar = CookieStorage::new();

Just inline auth_cache and cookie_jar below? That way the public and private cases are symmetric.

Comments from Reviewable

asajeffrey · 2016-06-10T17:07:11Z

./mach test-unit is failing:

$ ./mach test-unit
   Compiling util_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/util)
   Compiling profile_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/profile)
   Compiling style_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/style
   Compiling net_traits_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/net_traits)
   Compiling gfx_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/gfx)
   Compiling net_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/net)
   Compiling script_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/script)
   Compiling layout_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/layout)
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:44:21: 44:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:44     resource_thread.send(CoreResourceMsg::Exit(sender)).unwrap();
                                                                                        ^~~~
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:55:21: 55:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:55     resource_thread.send(CoreResourceMsg::Load(LoadData::new(LoadContext::Browsing, url, &ResourceTest),
                                                                                        ^~~~
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:63:21: 63:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:63     resource_thread.send(CoreResourceMsg::Exit(sender)).unwrap();
                                                                                        ^~~~
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:237:21: 237:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:237     resource_thread.send(CoreResourceMsg::Load(LoadData::new(LoadContext::Browsing, url, &ResourceTest),
                                                                                         ^~~~
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:242:21: 242:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:242     resource_thread.send(CoreResourceMsg::Cancel(res_id)).unwrap();
                                                                                         ^~~~
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:244:21: 244:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:244     resource_thread.send(CoreResourceMsg::Synchronize(sync_sender)).unwrap();
                                                                                         ^~~~
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:252:21: 252:25 error: no method named `send` found for type `(ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>, ipc_channel::ipc::IpcSender<net_traits::CoreResourceMsg>)` in the current scope
/home/travis/build/servo/servo/tests/unit/net/resource_thread.rs:252     resource_thread.send(CoreResourceMsg::Exit(exit_sender)).unwrap();
                                                                                         ^~~~
error: aborting due to 7 previous errors
Build failed, waiting for other jobs to finish...
error: Could not compile `net_tests`.
To learn more, run the command again with --verbose.
The command "./mach test-unit" exited with 101.

jdm · 2016-06-10T17:53:25Z

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 424 [r6] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

This change doesn't appear to have much to do with private browsing, care to comment?

I can't figure out what you're commenting about from Reviewable. Maybe you observed a change that was part of a rebase? The self.shutting_down check is already present on master.

components/net/resource_thread.rs, line 209 [r7] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Just inline auth_cache and cookie_jar below? That way the public and private cases are symmetric.

They're used in the if block that follows their declaration...

Comments from Reviewable

asajeffrey · 2016-06-10T18:17:09Z

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 424 [r6] (raw file):

Previously, jdm (Josh Matthews) wrote…

I can't figure out what you're commenting about from Reviewable. Maybe you observed a change that was part of a rebase? The self.shutting_down check is already present on master.

Ah, it's part of the rebase, cool. (In fact, I may have written that line...)

components/net/resource_thread.rs, line 209 [r7] (raw file):

Previously, jdm (Josh Matthews) wrote…

They're used in the if block that follows their declaration...

Oh, fair enough.

Comments from Reviewable

asajeffrey · 2016-06-10T18:18:40Z

After you get test-unit to pass and squash, you can r=me.

bors-servo · 2016-06-12T04:20:39Z

☔ The latest upstream changes (presumably #11556) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-06-20T18:19:27Z

@bors-servo: r=asajeffrey

bors-servo · 2016-06-20T18:19:28Z

📌 Commit 7ca7b03 has been approved by asajeffrey

bors-servo · 2016-06-20T18:41:49Z

⌛ Testing commit 7ca7b03 with merge 3f377aa...


        Auto merge of #11544 - jdm:privatebrowsing, r=asajeffrey

Implement private browsing for mozbrowser  Support the `mozprivatebrowsing` attribute on mozbrowser iframes. This separates the non-private and private sessions in terms of cookies, HSTS lists, cached HTTP credentials, HTTP connection pools, and web storage. The private session is shared between all private mozbrowsers, and lasts until shutdown. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11544)

bors-servo · 2016-06-20T18:44:35Z

💔 Test failed - linux-dev


        Propagate privacy information of iframes to corresponding pipelines. …

…Make iframes of differing privacy values be considered cross-origin. Make the constellation hand out separate private and public channels for the pipeline content to communicate with the resource thread as necessary.

jdm · 2016-06-20T18:55:34Z

@bors-servo: r=asajeffrey

bors-servo · 2016-06-20T18:55:35Z

📌 Commit 7e2b4d1 has been approved by asajeffrey

bors-servo · 2016-06-20T19:32:17Z

⌛ Testing commit 7e2b4d1 with merge 6166d8e...


        Auto merge of #11544 - jdm:privatebrowsing, r=asajeffrey

Implement private browsing for mozbrowser  Support the `mozprivatebrowsing` attribute on mozbrowser iframes. This separates the non-private and private sessions in terms of cookies, HSTS lists, cached HTTP credentials, HTTP connection pools, and web storage. The private session is shared between all private mozbrowsers, and lasts until shutdown. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11544)

bors-servo · 2016-06-20T20:28:20Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned asajeffrey Jun 1, 2016

highfive added the S-awaiting-review label Jun 1, 2016

jdm changed the title ~~Privatebrowsing~~ Implement private browsing for mozbrowser Jun 1, 2016

jdm mentioned this pull request Jun 1, 2016

Private browsing student project #10690

Closed

asajeffrey added S-awaiting-answer and removed S-awaiting-review labels Jun 1, 2016

paulrouget mentioned this pull request Jun 2, 2016

Consider supporting mozprivatebrowsing browserhtml/browserhtml#1071

Closed

nox mentioned this pull request Jun 2, 2016

Add an option to enable content blocker for a specific top level browsing context #11500

Open

highfive added the S-needs-rebase label Jun 4, 2016

jdm force-pushed the jdm:privatebrowsing branch from 82155b3 to e29b299 Jun 8, 2016

highfive added the S-awaiting-review label Jun 8, 2016

jdm removed S-awaiting-answer S-needs-rebase labels Jun 8, 2016

asajeffrey added S-awaiting-answer S-needs-squash and removed S-awaiting-review labels Jun 10, 2016

highfive added the S-needs-rebase label Jun 12, 2016

jdm force-pushed the jdm:privatebrowsing branch from e29b299 to a6f16ae Jun 13, 2016

highfive added the S-awaiting-review label Jun 13, 2016

jdm removed the S-awaiting-answer label Jun 13, 2016

highfive added the S-needs-rebase label Jun 14, 2016

jdm mentioned this pull request Jun 17, 2016

Implement contextual identity? #11777

Open

jdm force-pushed the jdm:privatebrowsing branch from a6f16ae to 56a97fe Jun 20, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jun 20, 2016

jdm force-pushed the jdm:privatebrowsing branch from 56a97fe to 7ca7b03 Jun 20, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jun 20, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 20, 2016

jdm force-pushed the jdm:privatebrowsing branch from 7ca7b03 to 7e2b4d1 Jun 20, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jun 20, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 20, 2016

bors-servo merged commit 7e2b4d1 into servo:master Jun 20, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 20, 2016

bors-servo mentioned this pull request Jun 20, 2016

Add webdriver commands for (Get|Add)Cookie #10826

Merged

servo / servo

Implement private browsing for mozbrowser #11544

Implement private browsing for mozbrowser #11544

jdm commented Jun 1, 2016 •

edited by larsbergstrom

highfive commented Jun 1, 2016

asajeffrey commented Jun 1, 2016

bors-servo commented Jun 4, 2016

asajeffrey commented Jun 4, 2016

asajeffrey commented Jun 10, 2016

asajeffrey commented Jun 10, 2016

jdm commented Jun 10, 2016

asajeffrey commented Jun 10, 2016

asajeffrey commented Jun 10, 2016

bors-servo commented Jun 12, 2016

jdm commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

jdm commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

servo / servo

Join GitHub today

Implement private browsing for mozbrowser #11544

Implement private browsing for mozbrowser #11544

Conversation

jdm commented Jun 1, 2016 • edited by larsbergstrom

highfive commented Jun 1, 2016

asajeffrey commented Jun 1, 2016

bors-servo commented Jun 4, 2016

asajeffrey commented Jun 4, 2016

asajeffrey commented Jun 10, 2016

asajeffrey commented Jun 10, 2016

jdm commented Jun 10, 2016

asajeffrey commented Jun 10, 2016

asajeffrey commented Jun 10, 2016

bors-servo commented Jun 12, 2016

jdm commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

jdm commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

jdm commented Jun 1, 2016 •

edited by larsbergstrom