986439 - [e10s] Don't allow nsNullPrincipalURI to be used for referrer, to stop serialisation error.

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bob Owen (:bobowen)]

Some of the iframe sandbox navigation tests fail with e10s.

This is because a null principal is used to enforce the sandboxed origin browsing context flag [1] and the URI of this gets passed as |aLastVisitedURI| in History::VisitURI.

With e10s an attempt is made to serialise this which crashes.

Possible patch to follow.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#sandboxed-origin-browsing-context-flag

Comment 1

[Bob Owen (:bobowen)]

Attachment: Allow nsNullPrincipalURI for nsNullPrincipal (with scheme moz-nullprincipal) to be serialised

This simply adds the "moz-nullprincipal" scheme to the allowed schemes array.

Comment 2

[Bob Owen (:bobowen)]

So, the change certainly makes the tests pass, as mozilla::ipc::SerializeURI no longer blows up, but, looking into it further, I'm not sure that it is the correct solution for two reasons:

1) While mozilla::ipc::SerializeURI "works", the corresponding mozilla::ipc::DeserializeURI deserialises the URI to an nsSimpleURI with a scheme of "moz-nullprincipal", so I'm not sure what problems this might cause.

2) Should an nsNullPrincipalURI be being passed as |aLastVisitedURI| to History::VisitURI in the first place?
This seems to happen because of the code in nsLocation::CheckURL.
The |principalURI| (being nsNullPrinicipalURI in the case of sandboxing) is different from the |docOriginalURI| and so the |principalURI| gets used as the referrer, which eventually ends up as |aLastVisitedURI|.
So, when it is nsNullPrincipalURI maybe the referrer should be left unset.
Or maybe it should be checked and nulled somewhere before in gets passed to VisitURI, nsDocShell::ExtractLastVisit seems like a good place. (I'll try this and see if it also works.)

For the actual referrer, I think nsNullPrincipalURI gets thrown out in HttpBaseChannel::SetReferrer, because it is not in the |referrerWhiteList|.

Comment 3

[Bob Owen (:bobowen)]

So ... changing nsDocShell::ExtractLastVisit fixes some of the sandboxing tests, but we also get an attempt to serialise an nsNullPrincipalURI because of (line 9929):

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=e15020347f1b&mark=9919-9931#9919

So, either we:
1) Fix up this and any other uses of nsNullPrincipalURI as the referrer.
2) Set the referrer to null in nsLocation::CheckURL when nsNullPrincipalURI.
2) Go with serialising nsNullPrincipalURI and maybe fix the deserialise.

or something else ... I haven't thought of.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Don't use nsNullPrincipalURI for documentURI or last visited URI v1

The crashes we discussed when I set a null referrer, were actually my fault. :)

So, I've fixed that and things seem to work.

I didn't add a function to principal to get the referrer URI, as given that we need to change this to use Source Browsing Context, it seemed like a lot of extra effort.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Enable tests that now pass with e10s v1

This just enables the tests that now work for e10s.

I think quite a lot of the docshell ones (if not all) were actually fixed by billm in bugs 978610 and 924260.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Don't use nsNullPrincipalURI for referrer v1

Sorry, hadn't updated the review comment.

See comment 4 for other details.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8398677 [details] [diff] [review]
Don't use nsNullPrincipalURI for referrer v1

So the goal is to nix this code getting referrers from principals eventually?  Maybe add comments at both callsites pointing to the bug that will do that?

r=me with that if you also add NS_SUCCEEDED() checks for the result of the SchemeIs call.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8398665 [details] [diff] [review]
Enable tests that now pass with e10s v1

r=more-test-coverage!

Comment 9

[Bob Owen (:bobowen)]

Attachment: Don't use nsNullPrincipalURI for referrer v2

r=bz - from comment 7 with issues addressed.

Thanks bz.

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8398677 [details] [diff] [review]
> Don't use nsNullPrincipalURI for referrer v1
> 
> So the goal is to nix this code getting referrers from principals
> eventually?  Maybe add comments at both callsites pointing to the bug that
> will do that?

Comment expanded including link to bug 960639.
 
> r=me with that if you also add NS_SUCCEEDED() checks for the result of the
> SchemeIs call.

Keep forgetting this, sorry.
NS_SUCCEEDED() added before checking out argument.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Enable tests that now pass with e10s v2

r=bz - from comment 8.

Thanks again bz.

Fairly extensive try push without e10s:
https://tbpl.mozilla.org/?tree=Try&rev=c5e06dc77801

The try push with e10s looked a bit scary:
https://tbpl.mozilla.org/?tree=Try&rev=000620935047

But when I compared it to an e10s push without any changes:
https://tbpl.mozilla.org/?tree=Try&rev=000620935047
The failures are fairly consistent and certainly the tests I have enabled are passing, apart from test_bug511449.html, which I have re-disabled.

I also noticed that tests in docshell/test/navigation were disabled.
Not all of these are now working, so I've removed the blanket disable and just disabled the individual tests that are failing.
Another small try push with these tests included:
https://tbpl.mozilla.org/?tree=Try&rev=e8ccbc115013

Comment 11

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #10)
> Created attachment 8399292 [details] [diff] [review]
> But when I compared it to an e10s push without any changes:
> https://tbpl.mozilla.org/?tree=Try&rev=000620935047
> The failures are fairly consistent and certainly the tests I have enabled

Sorry, e10s without any changes should have been:
https://tbpl.mozilla.org/?tree=Try&rev=7f099ee01208

Comment 12

[Bob Owen (:bobowen)]

Landing order:
https://bugzilla.mozilla.org/attachment.cgi?id=8399264
https://bugzilla.mozilla.org/attachment.cgi?id=8399292

Thanks

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Bob. This is really helpful!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eff086287a8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/66cc90755ae7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eff086287a8d
https://hg.mozilla.org/mozilla-central/rev/66cc90755ae7

Comment 16

[Firefox Commit Bot]

The following changesets are now in Firefox Nightly:

> eff086287a8d Bug 986439 - Don't use nsNullPrincipalURI for referrer. r=bz
> 66cc90755ae7 Bug 986439 - Enable tests that now pass with e10s. r=bz

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central