1225384 - Disallow nsResProtocolHandler::SetSubstitution changing the meaning of resource:///, resource://app/ and resource://gre/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mike Hommey [:glandium]]

See bug 1224000 comment 0

Comment 1

[Mike Hommey [:glandium]]

Attachment: Change how the default resource "host names" are handled

The meaning of resource:///, resource://app/ and resource://gre/ needs to
remain constant. Unfortunately, the model of the resource protocol handler
is that it is possible to set substitutions that change their meaning.

So, we forbid setting overwriting the substitutions for those three
special "host names".

Unfortunately, with e10s, the full list of substitutions is also sent to
the content process, which then sets substitutions, making it harder to
know in which cases SetSubstitution is valid for those three "host names"
or not.

So instead of trying to find the right heuristics, use the recently added
SubstitutingProtocolHandler::ResolveSpecialCases API to handle the three
"host names" instead of storing them as "normal" substitutions.

Still actively reject SetSubstitution with the three special "host names"
so as to find issues such as bug 1224000 instead of allowing the chrome
manifest entry and have it silently ignored.

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8688301 [details] [diff] [review]
Change how the default resource "host names" are handled

Review of attachment 8688301 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/res/nsResProtocolHandler.cpp
@@ +38,5 @@
> +    mGREURI.Truncate(mGREURI.Length() - 1);
> +    if (mAppURI.Length()) {
> +      mAppURI.Truncate(mAppURI.Length() - 1);
> +    } else {
> +      mGREURI = mAppURI;

Shouldn't this be mAppURI = mGREURI instead?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Michal Novotny (:michal) from comment #2)
> Comment on attachment 8688301 [details] [diff] [review]
> Change how the default resource "host names" are handled
> 
> Review of attachment 8688301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/res/nsResProtocolHandler.cpp
> @@ +38,5 @@
> > +    mGREURI.Truncate(mGREURI.Length() - 1);
> > +    if (mAppURI.Length()) {
> > +      mAppURI.Truncate(mAppURI.Length() - 1);
> > +    } else {
> > +      mGREURI = mAppURI;
> 
> Shouldn't this be mAppURI = mGREURI instead?

Err, indeed.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Change how the default resource "host names" are handled

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c63b932647

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/28b612e72ba1

Failing test run: https://treeherder.mozilla.org/logviewer.html#?job_id=18866342&repo=mozilla-inbound
Failing jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8916b45bc527

16:22:34     INFO -  TEST-START | netwerk/test/unit/test_bug580508.js
16:22:35  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_bug580508.js | xpcshell return code: 0
16:22:35     INFO -  TEST-INFO took 327ms
16:22:35     INFO -  >>>>>>>
16:22:35     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
16:22:35     INFO -  NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIResProtocolHandler.getSubstitution]
16:22:35     INFO -  run_test@/builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug580508.js:14:1
16:22:35     INFO -  _execute_test@/builds/slave/test/build/tests/xpcshell/head.js:530:7
16:22:35     INFO -  @-e:1:1
16:22:35     INFO -  <<<<<<<

Comment 7

[Mike Hommey [:glandium]]

Attachment: Additional patch, to fold when landing

Additionally, make GetSubstitution return the URIs for the three special
"host names" through GetSubstitutionInternal, replacing what was originally
added in bug 257162. Those changes from bug 257162 relied on the
resource:app string being handled by nsXREDirProvider::GetFile, but that was
removed in bug 620931, effectively making that code now in
GetSubstitutionInternal useless.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f26fb489885c

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f26fb489885c