Cleanup around LocalFrame initialization

Cleanup around LocalFrame initialization

* Add a ScriptForbiddenScope to FrameLoader::Init(), since script shouldn't ever run in Init(). Simplify some callers of Init() that assume Init() might detach the frame.
* Use WebLocalFrameImpl::InitializeCoreFrame in CreateProvisional(), resolving a FIXME there.

BUG=

Review-Url: https://codereview.chromium.org/2869203002
Cr-Commit-Position: refs/heads/master@{#472173}
Committed: https://chromium.googlesource.com/chromium/src/+/21bb521df1e2e311b05b07819b43e42a5db5c402

[Nate Chapin, 2017-05-09 21:44:04]

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

[commit-bot: I haz the power, 2017-05-09 21:45:04]

Dry run: CQ is trying da patch.

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

[Nate Chapin, 2017-05-09 22:55:59]

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

[commit-bot: I haz the power, 2017-05-09 22:56:48]

Dry run: 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-05-10 01:00:42]

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

[commit-bot: I haz the power, 2017-05-10 01:00:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Nate Chapin, 2017-05-10 16:32:52]

Description was changed from

==========
Add a ScriptForbiddenScope to FrameLoader::Init, it shouldn't ever happrn

Also, simplify logic that calls FrameLoader::Init and handles the hypothetical
script execution.

BUG=
==========

to

==========
Cleanup around LocalFrame initialization

* Add a ScriptForbiddenScope to FrameLoader::Init(), since script shouldn't ever
run in Init(). Simplify some callers of Init() that assume Init() might detach
the frame.
* Use WebLocalFrameImpl::InitializeCoreFrame in CreateProvisional(), resolving a
FIXME there.

BUG=
==========

[Nate Chapin, 2017-05-10 16:33:20]

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1563: // TODO(dcheng): This
block is very similar to initializeCoreFrame. Try to
InitializeCoreFrame() does a couple of things CreateProvisional didn't, but it
all looks benign to me.

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1639:
CHECK(frame_->Loader().StateMachine()->IsDisplayingInitialEmptyDocument());
CHECK might be overkill here, but given that risk of giving a universal access
SecurityOrigin to a a non-empty document if a navigation does somehow happen
inside Init(), it seems worth it to be cautious.

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:335: // FIXME: currently
this calls LocalFrame::init() on the created LocalFrame,
I don't think this comment was correct, given that initial empty documents are
*supposed* to be invisible to content/ (unless they get scripted and
DidAccessInitialDocument gets called). Certainly, navigation notifications
should be suppressed.

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:342: // dispatch. However,
This comment is now obsolete.

[Nate Chapin, 2017-05-11 21:23:53]

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

[Nate Chapin, 2017-05-11 21:23:54]

dcheng, what do you think of this?

[dcheng, 2017-05-11 21:36:25]

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1563: // TODO(dcheng): This
block is very similar to initializeCoreFrame. Try to
On 2017/05/10 16:33:20, Nate Chapin wrote:
> InitializeCoreFrame() does a couple of things CreateProvisional didn't, but it
> all looks benign to me.

I think my main concern with structuring it this way is that we're calling
Init() with an incorrect owner: won't this affect things like sandbox flags on
the initial empty document?

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634:
GetFrame()->Tree().SetName(name);
frame_ here as well for consistency?

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:342: // dispatch. However,
On 2017/05/10 16:33:20, Nate Chapin wrote:
> This comment is now obsolete.

Should we update the comment in LocalFrame::Init() as well?

[dcheng, 2017-05-12 15:54:41]

Actually I thought about this a bit more, and I don't think there's any time the
initial set of flags, origin, etc matter on the initial empty Document in a
provisional frame. So LGTM with comments addressed

[dcheng, 2017-05-12 18:06:38]

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

[dcheng, 2017-05-12 18:06:39]

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1563: // TODO(dcheng): This
block is very similar to initializeCoreFrame. Try to
On 2017/05/11 21:36:25, dcheng wrote:
> On 2017/05/10 16:33:20, Nate Chapin wrote:
> > InitializeCoreFrame() does a couple of things CreateProvisional didn't, but
it
> > all looks benign to me.
> 
> I think my main concern with structuring it this way is that we're calling
> Init() with an incorrect owner: won't this affect things like sandbox flags on
> the initial empty document?

Btw, I talked with Alex about this, and I think it'd probably be worth a comment
noting that this is an issue (we won't inherit the right set of things with the
DummyOwner), but that it's not an issue because the initial empty Document on a
provisional local frame is not reachable via script, and is essentially inert.
Since we do set the right FrameOwner just a few lines later, any subsequent
loads will "just do the right thing"

[Nate Chapin, 2017-05-12 19:52:29]

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1563: // TODO(dcheng): This
block is very similar to initializeCoreFrame. Try to
On 2017/05/12 18:06:38, dcheng wrote:
> On 2017/05/11 21:36:25, dcheng wrote:
> > On 2017/05/10 16:33:20, Nate Chapin wrote:
> > > InitializeCoreFrame() does a couple of things CreateProvisional didn't,
but
> it
> > > all looks benign to me.
> > 
> > I think my main concern with structuring it this way is that we're calling
> > Init() with an incorrect owner: won't this affect things like sandbox flags
on
> > the initial empty document?
> 
> Btw, I talked with Alex about this, and I think it'd probably be worth a
comment
> noting that this is an issue (we won't inherit the right set of things with
the
> DummyOwner), but that it's not an issue because the initial empty Document on
a
> provisional local frame is not reachable via script, and is essentially inert.
> Since we do set the right FrameOwner just a few lines later, any subsequent
> loads will "just do the right thing"

Done.

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1634:
GetFrame()->Tree().SetName(name);
On 2017/05/11 21:36:25, dcheng wrote:
> frame_ here as well for consistency?

Done.

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (left):

https://codereview.chromium.org/2869203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:342: // dispatch. However,
On 2017/05/11 21:36:25, dcheng wrote:
> On 2017/05/10 16:33:20, Nate Chapin wrote:
> > This comment is now obsolete.
> 
> Should we update the comment in LocalFrame::Init() as well?

Dropped it, it doesn't look like there's any substance besides the claim that
FrameLoader::Init() can detach the world.

[Nate Chapin, 2017-05-12 19:52:36]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-05-12 19:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2869203002/#ps40001
(title: "Address dcheng's comments")

[commit-bot: I haz the power, 2017-05-12 19:53:22]

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-05-12 19:55:37]

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

[commit-bot: I haz the power, 2017-05-12 19:55:38]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nate Chapin, 2017-05-12 20:19:26]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-05-12 20:19:53]

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-05-12 21:48:00]

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

[commit-bot: I haz the power, 2017-05-12 21:48:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2017-05-12 22:10:50]

Can we also run this through linux_site_isolation, just to be sure that layout
tests don't break with --site-per-process?

[Nate Chapin, 2017-05-15 22:48:51]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-05-15 22:48:51]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2869203002/#ps80001
(title: "+test fix")

[commit-bot: I haz the power, 2017-05-15 22:49:39]

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-05-16 00:47:09]

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

[commit-bot: I haz the power, 2017-05-16 00:47:11]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-05-16 16:45:24]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2017-05-16 16:46:09]

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-05-16 18:38:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494953124145420,
"parent_rev": "d82dbc620a69d659b7cdf4d105e62a8cb62156dc", "commit_rev":
"21bb521df1e2e311b05b07819b43e42a5db5c402"}

[commit-bot: I haz the power, 2017-05-16 18:38:22]

Description was changed from

==========
Cleanup around LocalFrame initialization

* Add a ScriptForbiddenScope to FrameLoader::Init(), since script shouldn't ever
run in Init(). Simplify some callers of Init() that assume Init() might detach
the frame.
* Use WebLocalFrameImpl::InitializeCoreFrame in CreateProvisional(), resolving a
FIXME there.

BUG=
==========

to

==========
Cleanup around LocalFrame initialization

* Add a ScriptForbiddenScope to FrameLoader::Init(), since script shouldn't ever
run in Init(). Simplify some callers of Init() that assume Init() might detach
the frame.
* Use WebLocalFrameImpl::InitializeCoreFrame in CreateProvisional(), resolving a
FIXME there.

BUG=

Review-Url: https://codereview.chromium.org/2869203002
Cr-Commit-Position: refs/heads/master@{#472173}
Committed:
https://chromium.googlesource.com/chromium/src/+/21bb521df1e2e311b05b07819b43...
==========

[commit-bot: I haz the power, 2017-05-16 18:38:23]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/21bb521df1e2e311b05b07819b43...