Don't set the origin twice when navigating for javascript: URLs

Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}

Committed: https://crrev.com/7fe934264200f6daf02881ceff76f578db0b6821
Cr-Commit-Position: refs/heads/master@{#374023}

[jochen (gone - plz use gerrit), 2016-02-05 14:45:02]

stole dcheng's tests

ptal

if you like it, please CQ+ - I'm OOO now

[dcheng, 2016-02-05 17:54:38]

Hmm...

If I understand correctly, with this patch, the security token for the v8
context gets set to the default security token and it's treated as a unique
origin.

The spec says this though:
"The origin is an alias to the origin of the active document of the browsing
context being navigated when the navigate algorithm was invoked."

So if this were perfectly correct, I think the security token should actually be
"127.0.0.1:8000", right?

[dcheng, 2016-02-05 18:46:11]

OK, I think I get it: this is OK because the default security token forces it to
fall back to canAccessFrame(). And we do end up setting the correct security
origin on the Document later in createWriterFor... so this turns out to be OK.

Overall, it still feels weird to me: determining whether or not to inherit based
on the URL seems kind of dangerous. There are also some other weird things here:
we temporarily have a unique security origin inside the installNewDocument()
call, and once the stack unwinds back into createWriterFor(), then we correct
it. Also, we plumb through ownerDocument (which is m_frame->document() but then
we use m_frame->document() directly in a number of places for handling
javascript URL. But I think we can examine that weirdness independently of this.

LGTM.

[Nate Chapin, 2016-02-05 21:28:36]

On 2016/02/05 18:46:11, dcheng wrote:
> OK, I think I get it: this is OK because the default security token forces it
to
> fall back to canAccessFrame(). And we do end up setting the correct security
> origin on the Document later in createWriterFor... so this turns out to be OK.
> 
> Overall, it still feels weird to me: determining whether or not to inherit
based
> on the URL seems kind of dangerous. There are also some other weird things
here:
> we temporarily have a unique security origin inside the installNewDocument()
> call, and once the stack unwinds back into createWriterFor(), then we correct
> it. Also, we plumb through ownerDocument (which is m_frame->document() but
then
> we use m_frame->document() directly in a number of places for handling
> javascript URL. But I think we can examine that weirdness independently of
this.
> 
> LGTM.

.5*LGTM, since my knowledge of origin handling is weak enough that I shouldn't
count for more. :)

I agree that inheriting based on URL is weird, but for better or worse it's not
the only place we special-case blank/empty urls, by a long shot.

I also agree that it's not ideal to have the temporary unique security origin,
but if there's no point where script can observe it, that's a wart we can live
with if need be.

[dcheng, 2016-02-05 21:32:32]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-02-05 21:35:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670173002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670173002/20001

[commit-bot: I haz the power, 2016-02-05 21:43:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-05 22:04:07]

Description was changed from

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org
==========

to

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}
==========

[commit-bot: I haz the power, 2016-02-05 22:04:08]

Patchset 2 (id:??) landed as
https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}

[sof, 2016-02-06 08:35:29]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-02-06 08:35:29]

https://codereview.chromium.org/1670173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DocumentInit.cpp (right):

https://codereview.chromium.org/1670173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DocumentInit.cpp:88: ,
m_shouldReuseDefaultView(other.m_shouldReuseDefaultView)
Missing initialization of m_shouldInheritSecurityOriginFromOwner

[sof, 2016-02-06 08:35:58]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1676793003/ by sigbjornf@opera.com.

The reason for reverting is: Number of MSan failures reported,
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....

[dcheng, 2016-02-06 08:45:57]

On 2016/02/06 at 08:35:58, sigbjornf wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1676793003/ by sigbjornf@opera.com.
> 
> The reason for reverting is: Number of MSan failures reported,
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....

sigbjornf, is it OK to reland as-is? It'll be easier to merge a fix to trunk
this way =/

[dcheng, 2016-02-06 08:46:06]

On 2016/02/06 at 08:45:57, dcheng wrote:
> On 2016/02/06 at 08:35:58, sigbjornf wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1676793003/ by sigbjornf@opera.com.
> > 
> > The reason for reverting is: Number of MSan failures reported,
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....
> 
> sigbjornf, is it OK to reland as-is? It'll be easier to merge a fix to trunk
this way =/

Err, to M48.

[sof, 2016-02-06 08:52:13]

On 2016/02/06 08:46:06, dcheng wrote:
> On 2016/02/06 at 08:45:57, dcheng wrote:
> > On 2016/02/06 at 08:35:58, sigbjornf wrote:
> > > A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1676793003/ by mailto:sigbjornf@opera.com.
> > > 
> > > The reason for reverting is: Number of MSan failures reported,
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....
> > 
> > sigbjornf, is it OK to reland as-is? It'll be easier to merge a fix to trunk
> this way =/
> 
> Err, to M48.

If reverting there is more overhead, then that's ok.

[dcheng, 2016-02-06 08:54:13]

On 2016/02/06 at 08:52:13, sigbjornf wrote:
> On 2016/02/06 08:46:06, dcheng wrote:
> > On 2016/02/06 at 08:45:57, dcheng wrote:
> > > On 2016/02/06 at 08:35:58, sigbjornf wrote:
> > > > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/1676793003/ by mailto:sigbjornf@opera.com.
> > > > 
> > > > The reason for reverting is: Number of MSan failures reported,
> >
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....
> > > 
> > > sigbjornf, is it OK to reland as-is? It'll be easier to merge a fix to
trunk
> > this way =/
> > 
> > Err, to M48.
> 
> If reverting there is more overhead, then that's ok.

It's going to require a respin either way, but I'm hoping to minimize the amount
of commits to M48 branch if we can even get this patch in... the stable cut was
9 hours ago =/

[sof, 2016-02-06 09:10:40]

On 2016/02/06 08:54:13, dcheng wrote:
> On 2016/02/06 at 08:52:13, sigbjornf wrote:
> > On 2016/02/06 08:46:06, dcheng wrote:
> > > On 2016/02/06 at 08:45:57, dcheng wrote:
> > > > On 2016/02/06 at 08:35:58, sigbjornf wrote:
> > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > https://codereview.chromium.org/1676793003/ by mailto:sigbjornf@opera.com.
> > > > > 
> > > > > The reason for reverting is: Number of MSan failures reported,
> > >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....
> > > > 
> > > > sigbjornf, is it OK to reland as-is? It'll be easier to merge a fix to
> trunk
> > > this way =/
> > > 
> > > Err, to M48.
> > 
> > If reverting there is more overhead, then that's ok.
> 
> It's going to require a respin either way, but I'm hoping to minimize the
amount
> of commits to M48 branch if we can even get this patch in... the stable cut
was
> 9 hours ago =/

As far as I can tell, this is the only thing MSan is picking up on, i.e., when
this copy constructor is invoked while constructing 'init' in
HTMLImportLoader::startWritingAndParsing().

[sof, 2016-02-06 09:12:03]

Description was changed from

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}
==========

to

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}
==========

[sof, 2016-02-06 09:12:53]

Relanding to expedite propagation of https://codereview.chromium.org/1676003002/

[sof, 2016-02-06 09:13:07]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-02-06 09:13:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670173002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670173002/20001

[commit-bot: I haz the power, 2016-02-06 09:17:33]

Description was changed from

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}
==========

to

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}
==========

[commit-bot: I haz the power, 2016-02-06 09:17:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-06 09:18:26]

Description was changed from

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}
==========

to

==========
Don't set the origin twice when navigating for javascript: URLs

As javascript: navigations will end up with a document that has the
original document's URL, we shouldn't run the algorithm to determine
origin on the original URL, especially since we'll override the origin
later anyways.

BUG=583445
R=japhet@chromium.org,dcheng@chromium.org,mkwst@chromium.org

Committed: https://crrev.com/75b27bda96f0fe77d40b502642d6669531981a49
Cr-Commit-Position: refs/heads/master@{#373917}

Committed: https://crrev.com/7fe934264200f6daf02881ceff76f578db0b6821
Cr-Commit-Position: refs/heads/master@{#374023}
==========

[commit-bot: I haz the power, 2016-02-06 09:18:27]

Patchset 2 (id:??) landed as
https://crrev.com/7fe934264200f6daf02881ceff76f578db0b6821
Cr-Commit-Position: refs/heads/master@{#374023}

[dcheng, 2016-02-09 01:13:29]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1678423003/ by dcheng@chromium.org.

The reason for reverting is: Breaks scrolling in some Google Sheets:
https://crbug.com/585091.