Mixed Content: Don't override a request's context during redirects.

Mixed Content: Don't override a request's context during redirects.

After https://codereview.chromium.org/605103003/, we have a request
context and frame type for incoming requests generated during redirects.
This patch removes the 'RequestContextInternal' override that was in
place, as it's now actively harmful.

The test was provided by mmaliszkiewicz@opera.com.

BUG=417841
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182940

[Mike West, 2014-09-26 09:50:37]

mkwst@chromium.org changed reviewers:
+ mmaliszkiewicz@opera.com, philipj@opera.com, sigbjornf@opera.com

[Mike West, 2014-09-26 09:50:37]

Hello, friendly Opera folks. Would you mind reviewing this fix for a bug
mmaliszkiewicz@opera.com reported? :)

It depends on the Chromium side landing first:
https://codereview.chromium.org/605103003/ I'll throw it to the bots once that
lands.

-mike

[philipj_slow, 2014-09-26 11:38:14]

http://code.google.com/p/chromium/issues/detail?id=417841 is 403 so I can't look
into the details, but the Chromium-side CL and this seems to add up, so LGTM.

[Mike West, 2014-09-26 11:50:03]

On 2014/09/26 11:38:14, philipj wrote:
> http://code.google.com/p/chromium/issues/detail?id=417841 is 403 so I can't
look
> into the details, but the Chromium-side CL and this seems to add up, so LGTM.

The bug says, basically: "Hey, Mike. You broke redirect checking for mixed
content." :)

[Mike West, 2014-09-26 11:50:14]

On 2014/09/26 11:50:03, Mike West wrote:
> On 2014/09/26 11:38:14, philipj wrote:
> > http://code.google.com/p/chromium/issues/detail?id=417841 is 403 so I can't
> look
> > into the details, but the Chromium-side CL and this seems to add up, so
LGTM.
> 
> The bug says, basically: "Hey, Mike. You broke redirect checking for mixed
> content." :)

Thanks for taking a look!

[mmal, 2014-09-26 12:10:41]

Hi!

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt
(right):

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt:1:
CONSOLE WARNING: Mixed Content: The page at
'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-script-through-redirection.html'
was loaded over HTTPS, but requested an insecure script
'http://127.0.0.1:8000/security/mixedContent/resources/script.js'. This content
should also be served over HTTPS.
Is it only a warning we now shows for scripts?

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection.html
(left):

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection.html:6:
testRunner.dumpAsText();
Somehow review system shows this as rename.

[Mike West, 2014-09-26 12:58:48]

Thanks!

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt
(right):

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt:1:
CONSOLE WARNING: Mixed Content: The page at
'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-script-through-redirection.html'
was loaded over HTTPS, but requested an insecure script
'http://127.0.0.1:8000/security/mixedContent/resources/script.js'. This content
should also be served over HTTPS.
On 2014/09/26 12:10:41, mmal wrote:
> Is it only a warning we now shows for scripts?

In layout tests, yes. Which is silly. I don't know the historical context, but
the "allow_running_of_insecure_content" flag is set to true in
TestPreferences::Reset. You have to add
`testRunner.overridePreference("WebKitAllowRunningInsecureContent", false);` to
the test to see the blocking behavior. *shrug*

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection.html
(left):

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection.html:6:
testRunner.dumpAsText();
On 2014/09/26 12:10:41, mmal wrote:
> Somehow review system shows this as rename.

Reuploaded with '--no-find-copies'. Git is weird. :)

[mmal, 2014-09-26 13:32:29]

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt
(right):

https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt:1:
CONSOLE WARNING: Mixed Content: The page at
'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-script-through-redirection.html'
was loaded over HTTPS, but requested an insecure script
'http://127.0.0.1:8000/security/mixedContent/resources/script.js'. This content
should also be served over HTTPS.
On 2014/09/26 12:58:48, Mike West wrote:
> On 2014/09/26 12:10:41, mmal wrote:
> > Is it only a warning we now shows for scripts?
> 
> In layout tests, yes. Which is silly. I don't know the historical context, but
> the "allow_running_of_insecure_content" flag is set to true in
> TestPreferences::Reset. You have to add
> `testRunner.overridePreference("WebKitAllowRunningInsecureContent", false);`
to
> the test to see the blocking behavior. *shrug*

Is it the case when only http server is started but the test is opened on chrome
canary? I saw warning there, while chrome stable blocked the script.

[Mike West, 2014-09-26 13:34:09]

On 2014/09/26 13:32:29, mmal wrote:
>
https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
> File
>
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt
> (right):
> 
>
https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
>
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt:1:
> CONSOLE WARNING: Mixed Content: The page at
>
'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-script-through-redirection.html'
> was loaded over HTTPS, but requested an insecure script
> 'http://127.0.0.1:8000/security/mixedContent/resources/script.js'. This
content
> should also be served over HTTPS.
> On 2014/09/26 12:58:48, Mike West wrote:
> > On 2014/09/26 12:10:41, mmal wrote:
> > > Is it only a warning we now shows for scripts?
> > 
> > In layout tests, yes. Which is silly. I don't know the historical context,
but
> > the "allow_running_of_insecure_content" flag is set to true in
> > TestPreferences::Reset. You have to add
> > `testRunner.overridePreference("WebKitAllowRunningInsecureContent", false);`
> to
> > the test to see the blocking behavior. *shrug*
> 
> Is it the case when only http server is started but the test is opened on
chrome
> canary? I saw warning there, while chrome stable blocked the script.

This is true only when you're running content shell with the
'--dump-render-tree' flag. It shouldn't be true for any build of
Chromium/Chrome.

[Mike West, 2014-09-26 13:34:34]

On 2014/09/26 13:34:09, Mike West wrote:
> On 2014/09/26 13:32:29, mmal wrote:
> >
>
https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
> > File
> >
>
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt
> > (right):
> > 
> >
>
https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
> >
>
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt:1:
> > CONSOLE WARNING: Mixed Content: The page at
> >
>
'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-script-through-redirection.html'
> > was loaded over HTTPS, but requested an insecure script
> > 'http://127.0.0.1:8000/security/mixedContent/resources/script.js'. This
> content
> > should also be served over HTTPS.
> > On 2014/09/26 12:58:48, Mike West wrote:
> > > On 2014/09/26 12:10:41, mmal wrote:
> > > > Is it only a warning we now shows for scripts?
> > > 
> > > In layout tests, yes. Which is silly. I don't know the historical context,
> but
> > > the "allow_running_of_insecure_content" flag is set to true in
> > > TestPreferences::Reset. You have to add
> > > `testRunner.overridePreference("WebKitAllowRunningInsecureContent",
false);`
> > to
> > > the test to see the blocking behavior. *shrug*
> > 
> > Is it the case when only http server is started but the test is opened on
> chrome
> > canary? I saw warning there, while chrome stable blocked the script.
> 
> This is true only when you're running content shell with the
> '--dump-render-tree' flag. It shouldn't be true for any build of
> Chromium/Chrome.

(If it is, then please file a bug and I will fix it ASAP.)

[mmal, 2014-09-26 13:40:00]

On 2014/09/26 13:34:34, Mike West wrote:
> On 2014/09/26 13:34:09, Mike West wrote:
> > On 2014/09/26 13:32:29, mmal wrote:
> > >
> >
>
https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
> > > File
> > >
> >
>
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/608733002/diff/20001/LayoutTests/http/tests/s...
> > >
> >
>
LayoutTests/http/tests/security/mixedContent/insecure-script-through-redirection-expected.txt:1:
> > > CONSOLE WARNING: Mixed Content: The page at
> > >
> >
>
'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-script-through-redirection.html'
> > > was loaded over HTTPS, but requested an insecure script
> > > 'http://127.0.0.1:8000/security/mixedContent/resources/script.js'. This
> > content
> > > should also be served over HTTPS.
> > > On 2014/09/26 12:58:48, Mike West wrote:
> > > > On 2014/09/26 12:10:41, mmal wrote:
> > > > > Is it only a warning we now shows for scripts?
> > > > 
> > > > In layout tests, yes. Which is silly. I don't know the historical
context,
> > but
> > > > the "allow_running_of_insecure_content" flag is set to true in
> > > > TestPreferences::Reset. You have to add
> > > > `testRunner.overridePreference("WebKitAllowRunningInsecureContent",
> false);`
> > > to
> > > > the test to see the blocking behavior. *shrug*
> > > 
> > > Is it the case when only http server is started but the test is opened on
> > chrome
> > > canary? I saw warning there, while chrome stable blocked the script.
> > 
> > This is true only when you're running content shell with the
> > '--dump-render-tree' flag. It shouldn't be true for any build of
> > Chromium/Chrome.
> 
> (If it is, then please file a bug and I will fix it ASAP.)

I described it in the first one. I can file another but after the weekend,
formally I'm OOO today.

[Mike West, 2014-09-27 06:44:45]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-09-27 06:45:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608733002/40001

[commit-bot: I haz the power, 2014-09-27 07:03:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-27 07:03:49]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[sof, 2014-09-28 18:55:48]

AssociatedURLLoaderTests unit tests seemingly in need of some WebURLRequest
resource context initializations, as they're now copied along on redirects.

[Mike West, 2014-09-28 20:40:09]

On 2014/09/28 18:55:48, sof wrote:
> AssociatedURLLoaderTests unit tests seemingly in need of some WebURLRequest
> resource context initializations, as they're now copied along on redirects.

Indeed. Something I plan to look at in the morning. :)

[Mike West, 2014-09-30 15:07:13]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-09-30 15:07:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608733002/40001

[commit-bot: I haz the power, 2014-09-30 16:59:02]

Committed patchset #3 (id:40001) as 182940