Closed Bug 986439 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bobowen, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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
This simply adds the "moz-nullprincipal" scheme to the allowed schemes array.
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|.
Blocks: e10s-tests
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.
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.
Attachment #8398664 - Flags: review?(bzbarsky)
Attachment #8394736 - Attachment is obsolete: true
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.
Attachment #8398665 - Flags: review?(bzbarsky)
Sorry, hadn't updated the review comment.

See comment 4 for other details.
Attachment #8398677 - Flags: review?(bzbarsky)
Attachment #8398664 - Attachment is obsolete: true
Attachment #8398664 - Flags: review?(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.
Attachment #8398677 - Flags: review?(bzbarsky) → review+
Comment on attachment 8398665 [details] [diff] [review]
Enable tests that now pass with e10s v1

r=more-test-coverage!
Attachment #8398665 - Flags: review?(bzbarsky) → review+
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.
Attachment #8398677 - Attachment is obsolete: true
Attachment #8399264 - Flags: review+
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
Attachment #8398665 - Attachment is obsolete: true
Attachment #8399292 - Flags: review+
(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
Landing order:
https://bugzilla.mozilla.org/attachment.cgi?id=8399264
https://bugzilla.mozilla.org/attachment.cgi?id=8399292

Thanks
Keywords: checkin-needed
Summary: [e10s] Allow nsNullPrincipalURI for nsNullPrincipal to be serialised, to fix issue with iframe sandbox. → [e10s] Don't allow nsNullPrincipalURI to be used for referrer, to stop serialisation error.
Thanks Bob. This is really helpful!
https://hg.mozilla.org/mozilla-central/rev/eff086287a8d
https://hg.mozilla.org/mozilla-central/rev/66cc90755ae7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
You need to log in before you can comment on or make changes to this bug.