Fix crash in frame owner property replication.

Fix crash in frame owner property replication.

Also updated the test that can trigger it, removing the unnecessary name
attributes that were masking the error.

BUG=705658
Review-Url: https://codereview.chromium.org/2775393003
Cr-Commit-Position: refs/heads/master@{#461111}
Committed: https://chromium.googlesource.com/chromium/src/+/aee9dcacf21624198c60857788c35906fffa97ad

[iclelland, 2017-03-27 20:12:47]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 20:13:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-03-27 20:14:39]

iclelland@chromium.org changed reviewers:
+ nasko@chromium.org

[iclelland, 2017-03-27 20:14:39]

+r nasko, can you PTAL? Thanks!

[nasko, 2017-03-27 21:23:21]

nasko@chromium.org changed reviewers:
+ alexmos@chromium.org

[nasko, 2017-03-27 21:23:22]

Adding alexmos@, who is the expert on replication state.

https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
File content/test/data/allowed_frames.html (left):

https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
content/test/data/allowed_frames.html:8: <iframe name="frame3" id="child-3"
allow="fullscreen vibrate" src="title1.html"></iframe>
Why are the names removed?

[commit-bot: I haz the power, 2017-03-28 00:03:56]

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

[commit-bot: I haz the power, 2017-03-28 00:03:57]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-03-28 00:50:18]

LGTM if my explanation about why the names are removed is correct.

https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
File content/test/data/allowed_frames.html (left):

https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
content/test/data/allowed_frames.html:8: <iframe name="frame3" id="child-3"
allow="fullscreen vibrate" src="title1.html"></iframe>
On 2017/03/27 21:23:22, nasko wrote:
> Why are the names removed?

I'm guessing this is because in InitRenderFrameProxy, we compare the
FrameOwnerProperties to the set of default values, to decide whether or not they
need to be sent for the new proxy.  With the frame names, the FOP::operator==
was concluding inequality before ever getting to compare the feature policy
vector.  Ian, can you confirm?

[iclelland, 2017-03-28 12:39:32]

On 2017/03/28 00:50:18, alexmos wrote:
> LGTM if my explanation about why the names are removed is correct.
> 
>
https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
> File content/test/data/allowed_frames.html (left):
> 
>
https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
> content/test/data/allowed_frames.html:8: <iframe name="frame3" id="child-3"
> allow="fullscreen vibrate" src="title1.html"></iframe>
> On 2017/03/27 21:23:22, nasko wrote:
> > Why are the names removed?
> 
> I'm guessing this is because in InitRenderFrameProxy, we compare the
> FrameOwnerProperties to the set of default values, to decide whether or not
they
> need to be sent for the new proxy.  With the frame names, the FOP::operator==
> was concluding inequality before ever getting to compare the feature policy
> vector.  Ian, can you confirm?

Yes, that's exactly right. As soon as I tried to test a real page in Chrome with
site isolation enabled, it crashed consistently.

I considered adding a specific SitePerProcessBrowserTest for just this case, but
since the name attribute isn't used anywhere in the test, the easiest way to
expose the failure was to remove it. (And also, the test would be rather tightly
bound to a should-be-irrelevant implementation detail.)

I can see about adding some unit tests for operator== that will test that each
change can independently set the return value to false, if there's value in
that.

[alexmos, 2017-03-28 18:34:18]

On 2017/03/28 12:39:32, iclelland wrote:
> On 2017/03/28 00:50:18, alexmos wrote:
> > LGTM if my explanation about why the names are removed is correct.
> > 
> >
>
https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
> > File content/test/data/allowed_frames.html (left):
> > 
> >
>
https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
> > content/test/data/allowed_frames.html:8: <iframe name="frame3" id="child-3"
> > allow="fullscreen vibrate" src="title1.html"></iframe>
> > On 2017/03/27 21:23:22, nasko wrote:
> > > Why are the names removed?
> > 
> > I'm guessing this is because in InitRenderFrameProxy, we compare the
> > FrameOwnerProperties to the set of default values, to decide whether or not
> they
> > need to be sent for the new proxy.  With the frame names, the
FOP::operator==
> > was concluding inequality before ever getting to compare the feature policy
> > vector.  Ian, can you confirm?
> 
> Yes, that's exactly right. As soon as I tried to test a real page in Chrome
with
> site isolation enabled, it crashed consistently.
> 
> I considered adding a specific SitePerProcessBrowserTest for just this case,
but
> since the name attribute isn't used anywhere in the test, the easiest way to
> expose the failure was to remove it. (And also, the test would be rather
tightly
> bound to a should-be-irrelevant implementation detail.)
> 
> I can see about adding some unit tests for operator== that will test that each
> change can independently set the return value to false, if there's value in
> that.

Exposing the crash by removing the names seems fine, but perhaps also add a
comment to the test file why this is important, so that someone won't later
re-add them for another test and lose this coverage?  A unit test would be fine
too and might be less fragile, but up to you if you want to add it here. 
(Aside, it'd also be nice if we could somehow guard against someone forgetting
to update operator== when adding a new attribute to FOP.)

[iclelland, 2017-03-31 03:27:07]

On 2017/03/28 18:34:18, alexmos wrote:
> On 2017/03/28 12:39:32, iclelland wrote:
> > On 2017/03/28 00:50:18, alexmos wrote:
> > > LGTM if my explanation about why the names are removed is correct.
> > > 
> > >
> >
>
https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
> > > File content/test/data/allowed_frames.html (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2775393003/diff/1/content/test/data/allowed_f...
> > > content/test/data/allowed_frames.html:8: <iframe name="frame3"
id="child-3"
> > > allow="fullscreen vibrate" src="title1.html"></iframe>
> > > On 2017/03/27 21:23:22, nasko wrote:
> > > > Why are the names removed?
> > > 
> > > I'm guessing this is because in InitRenderFrameProxy, we compare the
> > > FrameOwnerProperties to the set of default values, to decide whether or
not
> > they
> > > need to be sent for the new proxy.  With the frame names, the
> FOP::operator==
> > > was concluding inequality before ever getting to compare the feature
policy
> > > vector.  Ian, can you confirm?
> > 
> > Yes, that's exactly right. As soon as I tried to test a real page in Chrome
> with
> > site isolation enabled, it crashed consistently.
> > 
> > I considered adding a specific SitePerProcessBrowserTest for just this case,
> but
> > since the name attribute isn't used anywhere in the test, the easiest way to
> > expose the failure was to remove it. (And also, the test would be rather
> tightly
> > bound to a should-be-irrelevant implementation detail.)
> > 
> > I can see about adding some unit tests for operator== that will test that
each
> > change can independently set the return value to false, if there's value in
> > that.
> 
> Exposing the crash by removing the names seems fine, but perhaps also add a
> comment to the test file why this is important, so that someone won't later
> re-add them for another test and lose this coverage? 

Done. Thanks.

> A unit test would be fine too and might be less fragile, but up to you if you
want to add it here. 
> (Aside, it'd also be nice if we could somehow guard against someone forgetting
> to update operator== when adding a new attribute to FOP.)

I'll add a unit test in another CL, and think about the other issue. Perhaps we
can make the unit test guard partially against updating the comparator, if we
can make it such that you won't be able to add a new member without updating the
constructor calls in the test file as well. (Maybe.)

[iclelland, 2017-03-31 03:27:11]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-03-31 03:27:11]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2775393003/#ps20001
(title: "Add comment to test file")

[commit-bot: I haz the power, 2017-03-31 03:27:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 04:58:58]

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

[commit-bot: I haz the power, 2017-03-31 04:58:59]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[iclelland, 2017-03-31 12:53:12]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2017-03-31 12:53:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 14:08:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490964792742690,
"parent_rev": "4d49cf81d3e03c639b77275e78e91c535f1a05e7", "commit_rev":
"aee9dcacf21624198c60857788c35906fffa97ad"}

[commit-bot: I haz the power, 2017-03-31 14:09:06]

Description was changed from

==========
Fix crash in frame owner property replication.

Also updated the test that can trigger it, removing the unnecessary name
attributes that were masking the error.

BUG=705658
==========

to

==========
Fix crash in frame owner property replication.

Also updated the test that can trigger it, removing the unnecessary name
attributes that were masking the error.

BUG=705658

Review-Url: https://codereview.chromium.org/2775393003
Cr-Commit-Position: refs/heads/master@{#461111}
Committed:
https://chromium.googlesource.com/chromium/src/+/aee9dcacf21624198c60857788c3...
==========

[commit-bot: I haz the power, 2017-03-31 14:09:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/aee9dcacf21624198c60857788c3...