Exempt about: URLs from self-reference check for nested frames

Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the
self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion.

BUG=341858
Committed: https://crrev.com/c3fe8d78d15de7f5729a0ad6ef08f2e7bd3a1775
Cr-Commit-Position: refs/heads/master@{#362360}

[Xiaocheng, 2015-10-29 07:38:02]

Description was changed from

==========
Permit arbitrarily nested iframe srcdocs

In the scenario where an iframe whose srcdoc contains another iframe
with srcdoc, the inner frame was incorrectly deemed a self-reference
because it has the same URL "about:srcdoc" as the outer frame.

BUG=341858
==========

to

==========
Permit arbitrarily nested iframe srcdocs

In the scenario where an iframe whose srcdoc contains another iframe
with srcdoc, the inner frame was incorrectly deemed a self-reference
because it has the same URL "about:srcdoc" as the outer frame.

BUG=341858
==========

[Xiaocheng, 2015-10-29 07:38:03]

xiaochengh@chromium.org changed reviewers:
+ dcheng@chromium.org

[Xiaocheng, 2015-10-29 07:39:00]

PTAL.

[dcheng, 2015-10-29 14:47:08]

https://codereview.chromium.org/1410093006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1410093006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:758: if
(equalIgnoringFragmentIdentifier(document()->completeURL(AtomicString("about:srcdoc")),
url))
How about using Document::isSrcdocDocument() instead? Also, this should be
integrated into the for loop below: otherwise, you can use srcdoc to break the
self-reference checker.

[Xiaocheng, 2015-10-30 05:48:30]

Thanks for your comments.

- "How about using Document::isSrcdocDocument() instead?"

Hard-coding should indeed be avoided. For this purpose, some methods related to
about:srcdoc are added into KURL.h and KURL.cpp, which are analogous to those
for about:blank. They are also used in the self-reference check (in
LocalFrame.cpp) to avoid the hard-coding in Patch Set 1. Existing hard-codings
of "about:srcdoc" under the WebKit directory (2 occurrences, in FrameLoader.cpp
and HTMLFrameElementBase.cpp) are also avoided using these methods.

Unfortunately Document::isSrcdocDocument() cannot be used here, because when an
URL is being checked for self-reference, its document has not been loaded yet.

- "Also, this should be integrated into the for loop below: otherwise, you can
use srcdoc to break the self-reference checker."

I think srcdoc should always pass the self-reference check, so it should be
before the loop.


On 2015/10/29 14:47:08, dcheng wrote:
>
https://codereview.chromium.org/1410093006/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/1410093006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/LocalFrame.cpp:758: if
>
(equalIgnoringFragmentIdentifier(document()->completeURL(AtomicString("about:srcdoc")),
> url))
> How about using Document::isSrcdocDocument() instead? Also, this should be
> integrated into the for loop below: otherwise, you can use srcdoc to break the
> self-reference checker.

[dcheng, 2015-11-05 01:38:19]

So it appears about:blank iframes have the same problem:

var win = window;
var i = 0;
while (i++ < 10) {
  win.document.body.appendChild(win.document.createElement('iframe'));
  win = win[0];
}

So I think this fix should actually be more general. Also, how do IE/FF handle
this? I'm assuming they both handle this just fine (compared to Chrome), but
it's always good to confirm.

[Xiaocheng, 2015-11-06 02:26:09]

Running your script in FF's console generated a bunch of nested frames as
expected. In Chrome, only one frame was generated. 

I think all URLs with the about scheme should be exempted from the
self-reference check, as they cannot be the source of infinitely recursive
frames. 

On 2015/11/05 01:38:19, dcheng wrote:
> So it appears about:blank iframes have the same problem:
> 
> var win = window;
> var i = 0;
> while (i++ < 10) {
>   win.document.body.appendChild(win.document.createElement('iframe'));
>   win = win[0];
> }
> 
> So I think this fix should actually be more general. Also, how do IE/FF handle
> this? I'm assuming they both handle this just fine (compared to Chrome), but
> it's always good to confirm.

[dcheng, 2015-11-10 23:41:20]

On 2015/11/06 at 02:26:09, xiaochengh wrote:
> Running your script in FF's console generated a bunch of nested frames as
expected. In Chrome, only one frame was generated. 
> 
> I think all URLs with the about scheme should be exempted from the
self-reference check, as they cannot be the source of infinitely recursive
frames. 
> 
> On 2015/11/05 01:38:19, dcheng wrote:
> > So it appears about:blank iframes have the same problem:
> > 
> > var win = window;
> > var i = 0;
> > while (i++ < 10) {
> >   win.document.body.appendChild(win.document.createElement('iframe'));
> >   win = win[0];
> > }
> > 
> > So I think this fix should actually be more general. Also, how do IE/FF
handle
> > this? I'm assuming they both handle this just fine (compared to Chrome), but
> > it's always good to confirm.

Yeah, that seems like the right thing to do.

[Xiaocheng, 2015-11-12 06:04:57]

This patch set is functionally the same as before but is simpler. Now it
contains only the about URL exemption from the self-reference check and new
layout tests.

The support for the KURL constant about:srcdoc is no longer relevant to this
patch. It is removed from this patch and will be moved to a new one.

Hope it looks good to you.

On 2015/11/10 23:41:20, dcheng wrote:
> On 2015/11/06 at 02:26:09, xiaochengh wrote:
> > Running your script in FF's console generated a bunch of nested frames as
> expected. In Chrome, only one frame was generated. 
> > 
> > I think all URLs with the about scheme should be exempted from the
> self-reference check, as they cannot be the source of infinitely recursive
> frames. 
> > 
> > On 2015/11/05 01:38:19, dcheng wrote:
> > > So it appears about:blank iframes have the same problem:
> > > 
> > > var win = window;
> > > var i = 0;
> > > while (i++ < 10) {
> > >   win.document.body.appendChild(win.document.createElement('iframe'));
> > >   win = win[0];
> > > }
> > > 
> > > So I think this fix should actually be more general. Also, how do IE/FF
> handle
> > > this? I'm assuming they both handle this just fine (compared to Chrome),
but
> > > it's always good to confirm.
> 
> Yeah, that seems like the right thing to do.

[dcheng, 2015-11-18 03:23:19]

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html
(right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:14:
frames.forEach( function(value) { assert_not_equals(value.clientHeight, 0) } )
How come this can't use a check for contentDocument?

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:15: },
"all nested about:blank iframes should be displayed correctly");
Nit: "nested about:blank iframes should not be blocked as self-referential
frames."

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/frames/srcdoc/srcdoc-deep-nested-frames.html
(right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/srcdoc/srcdoc-deep-nested-frames.html:16:
test(function() {assert_not_equals(frame3.contentDocument, null)}, "deep nested
frames should still have non-null content documents.");
Nit: "nested srcdocs should not be blocked as self-referential frames." might be
more descriptive.

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:757: // Exempt about URLs
from self-reference check.
Nit: s/about/about:/

[Xiaocheng, 2015-11-18 10:26:00]

Thanks for your comments.

I rewrote the two tests to better fit the coding style.

As having a non-null contentDocument does not necessarily mean the iframe is
displayed (see the detailed comments for reason), I also modified the criteria
in checking whether an iframe is displayed or not.

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html
(right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:14:
frames.forEach( function(value) { assert_not_equals(value.clientHeight, 0) } )
On 2015/11/18 03:23:18, dcheng wrote:
> How come this can't use a check for contentDocument?

TL;DR: For a nested frame created in this way, its contentDocument is always
non-null as soon as appendChild is executed, regardless of whether it is
displayed. 

The contentDocument becomes non-null after appendChild is called. I digged a
little bit into the implementation of appendChild, which does the following:
1. Modify the DOM tree.
2. Call HTMLFrameElementBase::isURLAllowed to checks if the frame's URL is
allowed. This method returns true immediately if the URL is the empty string,
and otherwise calls the self-reference checker LocalFrame::isURLAllowed.
3. If the URL is an empty string, replace it by "about:blank".
4. If the previous step returns true, load the contentDocument.
5. Set the LayoutObject for the contentDocument. This step calls
LocalFrame::isURLAllowed again, and aborts if it gets false.

createElement('iframe') returns an iframe whose URL is the empty string instead
of "about:blank". Therefore, in the execution of appendChild, it passes step 2
and gets a non-null contentDocument in step 4. However, since its URL has
already become "about:blank" in step 5, it fails the self-reference checker, and
hence, is not displayed.

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:15: },
"all nested about:blank iframes should be displayed correctly");
On 2015/11/18 03:23:18, dcheng wrote:
> Nit: "nested about:blank iframes should not be blocked as self-referential
> frames."

Done.

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/frames/srcdoc/srcdoc-deep-nested-frames.html
(right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/srcdoc/srcdoc-deep-nested-frames.html:16:
test(function() {assert_not_equals(frame3.contentDocument, null)}, "deep nested
frames should still have non-null content documents.");
On 2015/11/18 03:23:18, dcheng wrote:
> Nit: "nested srcdocs should not be blocked as self-referential frames." might
be
> more descriptive.

Done.

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:757: // Exempt about URLs
from self-reference check.
On 2015/11/18 03:23:19, dcheng wrote:
> Nit: s/about/about:/

Done.

[dcheng, 2015-11-18 21:32:51]

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html
(right):

https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:14:
frames.forEach( function(value) { assert_not_equals(value.clientHeight, 0) } )
On 2015/11/18 at 10:26:00, Xiaocheng wrote:
> On 2015/11/18 03:23:18, dcheng wrote:
> > How come this can't use a check for contentDocument?
> 
> TL;DR: For a nested frame created in this way, its contentDocument is always
non-null as soon as appendChild is executed, regardless of whether it is
displayed. 
> 
> The contentDocument becomes non-null after appendChild is called. I digged a
little bit into the implementation of appendChild, which does the following:
> 1. Modify the DOM tree.
> 2. Call HTMLFrameElementBase::isURLAllowed to checks if the frame's URL is
allowed. This method returns true immediately if the URL is the empty string,
and otherwise calls the self-reference checker LocalFrame::isURLAllowed.
> 3. If the URL is an empty string, replace it by "about:blank".
> 4. If the previous step returns true, load the contentDocument.
> 5. Set the LayoutObject for the contentDocument. This step calls
LocalFrame::isURLAllowed again, and aborts if it gets false.
> 
> createElement('iframe') returns an iframe whose URL is the empty string
instead of "about:blank". Therefore, in the execution of appendChild, it passes
step 2 and gets a non-null contentDocument in step 4. However, since its URL has
already become "about:blank" in step 5, it fails the self-reference checker, and
hence, is not displayed.

Hmm. I think there's some room for cleanup in the code here.

We have:
- HTMLFrameElementBase::isURLAllowed which does an empty string check and
javascript: URL check, and then calls LocalFrame::isURLAllowed.
- HTMLPlugInElement::allowedToLoadFrameURL which just does a javascript: URL
check, and then calls LocalFrame::isURLAllowed.
- LocalFrame::isURLAllowed which is just the self-reference check (and now
about: protocol) check.

Can we simplify this so it's not so weird?

[Xiaocheng, 2015-11-19 01:14:32]

On 2015/11/18 21:32:51, dcheng wrote:
>
https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html
> (right):
> 
>
https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:14:
> frames.forEach( function(value) { assert_not_equals(value.clientHeight, 0) } )
> On 2015/11/18 at 10:26:00, Xiaocheng wrote:
> > On 2015/11/18 03:23:18, dcheng wrote:
> > > How come this can't use a check for contentDocument?
> > 
> > TL;DR: For a nested frame created in this way, its contentDocument is always
> non-null as soon as appendChild is executed, regardless of whether it is
> displayed. 
> > 
> > The contentDocument becomes non-null after appendChild is called. I digged a
> little bit into the implementation of appendChild, which does the following:
> > 1. Modify the DOM tree.
> > 2. Call HTMLFrameElementBase::isURLAllowed to checks if the frame's URL is
> allowed. This method returns true immediately if the URL is the empty string,
> and otherwise calls the self-reference checker LocalFrame::isURLAllowed.
> > 3. If the URL is an empty string, replace it by "about:blank".
> > 4. If the previous step returns true, load the contentDocument.
> > 5. Set the LayoutObject for the contentDocument. This step calls
> LocalFrame::isURLAllowed again, and aborts if it gets false.
> > 
> > createElement('iframe') returns an iframe whose URL is the empty string
> instead of "about:blank". Therefore, in the execution of appendChild, it
passes
> step 2 and gets a non-null contentDocument in step 4. However, since its URL
has
> already become "about:blank" in step 5, it fails the self-reference checker,
and
> hence, is not displayed.
> 
> Hmm. I think there's some room for cleanup in the code here.
> 
> We have:
> - HTMLFrameElementBase::isURLAllowed which does an empty string check and
> javascript: URL check, and then calls LocalFrame::isURLAllowed.
> - HTMLPlugInElement::allowedToLoadFrameURL which just does a javascript: URL
> check, and then calls LocalFrame::isURLAllowed.
> - LocalFrame::isURLAllowed which is just the self-reference check (and now
> about: protocol) check.
> 
> Can we simplify this so it's not so weird?

Yes, the URL checking code is indeed kind of messy.

Maybe we should leave this patch simple and do the cleanup work in a separate
patch?

My apologies for being inactive in identifying smelly code and asking such
immature questions. As a Noogler who just started last month, I'm still trying
to learn the way how other people work...

[Xiaocheng, 2015-11-19 07:59:12]

On 2015/11/19 01:14:32, Xiaocheng wrote:
> On 2015/11/18 21:32:51, dcheng wrote:
> >
>
https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/1410093006/diff/60001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/fast/frames/deep-nested-about-blank.html:14:
> > frames.forEach( function(value) { assert_not_equals(value.clientHeight, 0) }
)
> > On 2015/11/18 at 10:26:00, Xiaocheng wrote:
> > > On 2015/11/18 03:23:18, dcheng wrote:
> > > > How come this can't use a check for contentDocument?
> > > 
> > > TL;DR: For a nested frame created in this way, its contentDocument is
always
> > non-null as soon as appendChild is executed, regardless of whether it is
> > displayed. 
> > > 
> > > The contentDocument becomes non-null after appendChild is called. I digged
a
> > little bit into the implementation of appendChild, which does the following:
> > > 1. Modify the DOM tree.
> > > 2. Call HTMLFrameElementBase::isURLAllowed to checks if the frame's URL is
> > allowed. This method returns true immediately if the URL is the empty
string,
> > and otherwise calls the self-reference checker LocalFrame::isURLAllowed.
> > > 3. If the URL is an empty string, replace it by "about:blank".
> > > 4. If the previous step returns true, load the contentDocument.
> > > 5. Set the LayoutObject for the contentDocument. This step calls
> > LocalFrame::isURLAllowed again, and aborts if it gets false.
> > > 
> > > createElement('iframe') returns an iframe whose URL is the empty string
> > instead of "about:blank". Therefore, in the execution of appendChild, it
> passes
> > step 2 and gets a non-null contentDocument in step 4. However, since its URL
> has
> > already become "about:blank" in step 5, it fails the self-reference checker,
> and
> > hence, is not displayed.
> > 
> > Hmm. I think there's some room for cleanup in the code here.
> > 
> > We have:
> > - HTMLFrameElementBase::isURLAllowed which does an empty string check and
> > javascript: URL check, and then calls LocalFrame::isURLAllowed.
> > - HTMLPlugInElement::allowedToLoadFrameURL which just does a javascript: URL
> > check, and then calls LocalFrame::isURLAllowed.
> > - LocalFrame::isURLAllowed which is just the self-reference check (and now
> > about: protocol) check.
> > 
> > Can we simplify this so it's not so weird?
> 
> Yes, the URL checking code is indeed kind of messy.
> 
> Maybe we should leave this patch simple and do the cleanup work in a separate
> patch?
> 
> My apologies for being inactive in identifying smelly code and asking such
> immature questions. As a Noogler who just started last month, I'm still trying
> to learn the way how other people work...

The code doesn't look good, but I don't have a good idea how to refactor,
either...

The JS checks in HTMLFrameElementBase::isURLAllowed and
HTMLPlugInElement::allowedToLoadFrameURL have different logic, as one is
resolved by ScriptController::canAccessFromCurrentOrigin, and the other is by
SecurityOrigin::canAccess. I think they should be left in their current forms.

The empty string check in HTMLFrameElementBase::isURLAllowed seems to be a trick
for speeding up the loading of empty frames from HTMLFrameElementBase::openURL,
but I'm not completely sure about this as there are still other call sites of
HTMLFrameElementBase::isURLAllowed. So I prefer to leave it here.

The about protocol check is part of self-reference check, so it should be placed
inside LocalFrame::isURLAllowed.

Now there is nothing left for refactoring.

[dcheng, 2015-12-01 04:21:02]

LGTM, but update the CL description since this handles it for all about: pages
now.

And it's probably worth describing why it's OK to skip the self-reference check
in this case as well.

[Xiaocheng, 2015-12-01 04:49:57]

Description was changed from

==========
Permit arbitrarily nested iframe srcdocs

In the scenario where an iframe whose srcdoc contains another iframe
with srcdoc, the inner frame was incorrectly deemed a self-reference
because it has the same URL "about:srcdoc" as the outer frame.

BUG=341858
==========

to

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the self-reference
check prevents the loading/rendering of a frame if it has the same URL as any of
its ancestor frames.

However, about: URLs should be exempted since they are reserved for other
purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

[Xiaocheng, 2015-12-01 04:50:36]

Description was changed from

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the self-reference
check prevents the loading/rendering of a frame if it has the same URL as any of
its ancestor frames.

However, about: URLs should be exempted since they are reserved for other
purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

to

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion,
the self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

[Xiaocheng, 2015-12-01 04:51:04]

Description was changed from

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion,
the self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

to

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the
self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

[Xiaocheng, 2015-12-01 04:51:26]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2015-12-01 04:52:31]

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

[commit-bot: I haz the power, 2015-12-01 06:18:11]

Description was changed from

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the
self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

to

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the
self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

[commit-bot: I haz the power, 2015-12-01 06:18:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-01 06:18:49]

Description was changed from

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the
self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion. 

BUG=341858
==========

to

==========
Exempt about: URLs from self-reference check for nested frames

When there are nested frames, to avoid infinite recursion, the
self-reference check prevents the loading/rendering of a frame
if it has the same URL as any of its ancestor frames.

However, about: URLs should be exempted since they are reserved
for other purposes and cannot be the source of infinite recursion.

BUG=341858

Committed: https://crrev.com/c3fe8d78d15de7f5729a0ad6ef08f2e7bd3a1775
Cr-Commit-Position: refs/heads/master@{#362360}
==========

[commit-bot: I haz the power, 2015-12-01 06:18:50]

Patchset 5 (id:??) landed as
https://crrev.com/c3fe8d78d15de7f5729a0ad6ef08f2e7bd3a1775
Cr-Commit-Position: refs/heads/master@{#362360}