Call EnsureWebKitInitialized() before registering extensions

Call EnsureWebKitInitialized() before registering extensions

Blink needs to be initialized before registering an extension because:
- WebScriptController::registerExtension() allocates an WTF::Vector on the first
  call.
- WTF::Vector uses PartitionAlloc.
- PartitionAlloc's partitions needs to be initialized before allocating memory.
  Blink initialization does the job.

Before this CL, partitions are initialized lazily but lazy initialization
doesn't provide proper histogram function, which might be the cause of missing
report for PartitionAlloc.CommittedSize UMA.

BUG=501171
Committed: https://crrev.com/0d244fa5e106b3d146655a2c623539a3e1900831
Cr-Commit-Position: refs/heads/master@{#339200}

[bashi, 2015-06-17 03:18:14]

bashi@chromium.org changed reviewers:
+ sievers@chromium.org

[bashi, 2015-06-17 03:18:14]

PTAL?

`git cl upload` suggested you as a reviewer but feel free to add reviewers.

[haraken, 2015-06-17 03:49:36]

Thanks for looking into the issue. LGTM.

[no sievers, 2015-06-17 19:46:28]

lgtm

[bashi, 2015-06-18 22:51:16]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-06-18 22:53:42]

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

[commit-bot: I haz the power, 2015-06-19 00:18:38]

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

[commit-bot: I haz the power, 2015-06-19 00:18:40]

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

[bashi, 2015-06-19 01:57:42]

bashi@chromium.org changed reviewers:
+ paulmeyer@chromium.org

[bashi, 2015-06-19 02:02:24]

On 2015/06/19 00:18:40, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Hmm, I got an exception when I ran extensions_browsertests
--gtest_filter='AppViewTest.TestAppViewGoodDataShouldSucceed'.

[20377:20377:0619/105152:454387819124:INFO:CONSOLE(55)] "Uncaught TypeError:
AppView is not a function", source:
chrome-extension://naaenhbhcdbpibkeenmhfiaghedbaepm/main.js (55)

It seems that "AppView" is installed by
extensions/renderer/resources/guest_view/app_view/app_view.js. I have no idea
why calling EnsureWebKitInitialized() in RenderThreadImpl::RegisterExtension()
make it unavailable.

paulmeyer@, do you have any idea? `git log` shows that you touched app_view.js
several times.

[paulmeyer, 2015-06-19 13:21:14]

On 2015/06/19 02:02:24, bashi1 wrote:
> On 2015/06/19 00:18:40, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
> (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Hmm, I got an exception when I ran extensions_browsertests
> --gtest_filter='AppViewTest.TestAppViewGoodDataShouldSucceed'.
> 
> [20377:20377:0619/105152:454387819124:INFO:CONSOLE(55)] "Uncaught TypeError:
> AppView is not a function", source:
> chrome-extension://naaenhbhcdbpibkeenmhfiaghedbaepm/main.js (55)
> 
> It seems that "AppView" is installed by
> extensions/renderer/resources/guest_view/app_view/app_view.js. I have no idea
> why calling EnsureWebKitInitialized() in RenderThreadImpl::RegisterExtension()
> make it unavailable.
> 
> paulmeyer@, do you have any idea? `git log` shows that you touched app_view.js
> several times.

I'm not sure exactly what is happening, but it looks like lots of things are
going wrong before it even gets to where it can't recognize AppView. What is
happening with AppView is likely that app_view.js was just never run, since that
is where the AppView function is defined. Something must have gone wrong before
the dispatcher was able to run that file.
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

[bashi, 2015-07-08 08:24:19]

On 2015/06/19 13:21:14, Paul Meyer wrote:
> On 2015/06/19 02:02:24, bashi1 wrote:
> > On 2015/06/19 00:18:40, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
> > (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > Hmm, I got an exception when I ran extensions_browsertests
> > --gtest_filter='AppViewTest.TestAppViewGoodDataShouldSucceed'.
> > 
> > [20377:20377:0619/105152:454387819124:INFO:CONSOLE(55)] "Uncaught TypeError:
> > AppView is not a function", source:
> > chrome-extension://naaenhbhcdbpibkeenmhfiaghedbaepm/main.js (55)
> > 
> > It seems that "AppView" is installed by
> > extensions/renderer/resources/guest_view/app_view/app_view.js. I have no
idea
> > why calling EnsureWebKitInitialized() in
RenderThreadImpl::RegisterExtension()
> > make it unavailable.
> > 
> > paulmeyer@, do you have any idea? `git log` shows that you touched
app_view.js
> > several times.
> 
> I'm not sure exactly what is happening, but it looks like lots of things are
> going wrong before it even gets to where it can't recognize AppView. What is
> happening with AppView is likely that app_view.js was just never run, since
that
> is where the AppView function is defined. Something must have gone wrong
before
> the dispatcher was able to run that file.
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

Sorry for the late response. Took a close look and found the cause.

To make sure that "AppView" is installed, we need to add an
extensions::Dispatcher as a RenderProcessObserver via
RenderThreadImpl::AddObserver() *before* EnsureWebKitInitialized() is called.
OTOH, extensions::Dispatcher calls RenderTHreadImpl::RegisterExtension() in its
constructor. If we call EnsureWebKitInitialized() in RegisterExtension(),
creating a Dispatcher invokes EnsureWebKitInitialized() without adding
Dispatcher as a RenderProcessObserver and "AppView" won't be installed.

I think the right fix is to move RegisterExtension() call from
Dispatcher::Dispatcher() to Dispatcher::WebKitInitialized(), given that
RegisterExtension() needs blink to be initialized beforehand. WDYT?

[bashi, 2015-07-08 08:52:33]

On 2015/07/08 08:24:19, bashi1 wrote:
> On 2015/06/19 13:21:14, Paul Meyer wrote:
> > On 2015/06/19 02:02:24, bashi1 wrote:
> > > On 2015/06/19 00:18:40, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
> > > (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > > 
> > > Hmm, I got an exception when I ran extensions_browsertests
> > > --gtest_filter='AppViewTest.TestAppViewGoodDataShouldSucceed'.
> > > 
> > > [20377:20377:0619/105152:454387819124:INFO:CONSOLE(55)] "Uncaught
TypeError:
> > > AppView is not a function", source:
> > > chrome-extension://naaenhbhcdbpibkeenmhfiaghedbaepm/main.js (55)
> > > 
> > > It seems that "AppView" is installed by
> > > extensions/renderer/resources/guest_view/app_view/app_view.js. I have no
> idea
> > > why calling EnsureWebKitInitialized() in
> RenderThreadImpl::RegisterExtension()
> > > make it unavailable.
> > > 
> > > paulmeyer@, do you have any idea? `git log` shows that you touched
> app_view.js
> > > several times.
> > 
> > I'm not sure exactly what is happening, but it looks like lots of things are
> > going wrong before it even gets to where it can't recognize AppView. What is
> > happening with AppView is likely that app_view.js was just never run, since
> that
> > is where the AppView function is defined. Something must have gone wrong
> before
> > the dispatcher was able to run that file.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> Sorry for the late response. Took a close look and found the cause.
> 
> To make sure that "AppView" is installed, we need to add an
> extensions::Dispatcher as a RenderProcessObserver via
> RenderThreadImpl::AddObserver() *before* EnsureWebKitInitialized() is called.
> OTOH, extensions::Dispatcher calls RenderTHreadImpl::RegisterExtension() in
its
> constructor. If we call EnsureWebKitInitialized() in RegisterExtension(),
> creating a Dispatcher invokes EnsureWebKitInitialized() without adding
> Dispatcher as a RenderProcessObserver and "AppView" won't be installed.
> 
> I think the right fix is to move RegisterExtension() call from
> Dispatcher::Dispatcher() to Dispatcher::WebKitInitialized(), given that
> RegisterExtension() needs blink to be initialized beforehand. WDYT?

Many browser tests are failing...

[18265:18265:0708/012732:6681709439:FATAL:safe_builtins.cc(141)] Check failed:
!value.IsEmpty() && value->IsObject(). Array

It seems that SafeBuiltins() should be registered before
EnsureWebKitInitialized(), But this sounds strange to me because
RenderThreadImpl::RegisterExtensions() uses a blink public API
(WebScriptController::registerExtension).

[paulmeyer, 2015-07-08 13:35:02]

On 2015/07/08 08:24:19, bashi1 wrote:
> On 2015/06/19 13:21:14, Paul Meyer wrote:
> > On 2015/06/19 02:02:24, bashi1 wrote:
> > > On 2015/06/19 00:18:40, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
> > > (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > > 
> > > Hmm, I got an exception when I ran extensions_browsertests
> > > --gtest_filter='AppViewTest.TestAppViewGoodDataShouldSucceed'.
> > > 
> > > [20377:20377:0619/105152:454387819124:INFO:CONSOLE(55)] "Uncaught
TypeError:
> > > AppView is not a function", source:
> > > chrome-extension://naaenhbhcdbpibkeenmhfiaghedbaepm/main.js (55)
> > > 
> > > It seems that "AppView" is installed by
> > > extensions/renderer/resources/guest_view/app_view/app_view.js. I have no
> idea
> > > why calling EnsureWebKitInitialized() in
> RenderThreadImpl::RegisterExtension()
> > > make it unavailable.
> > > 
> > > paulmeyer@, do you have any idea? `git log` shows that you touched
> app_view.js
> > > several times.
> > 
> > I'm not sure exactly what is happening, but it looks like lots of things are
> > going wrong before it even gets to where it can't recognize AppView. What is
> > happening with AppView is likely that app_view.js was just never run, since
> that
> > is where the AppView function is defined. Something must have gone wrong
> before
> > the dispatcher was able to run that file.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> Sorry for the late response. Took a close look and found the cause.
> 
> To make sure that "AppView" is installed, we need to add an
> extensions::Dispatcher as a RenderProcessObserver via
> RenderThreadImpl::AddObserver() *before* EnsureWebKitInitialized() is called.
> OTOH, extensions::Dispatcher calls RenderTHreadImpl::RegisterExtension() in
its
> constructor. If we call EnsureWebKitInitialized() in RegisterExtension(),
> creating a Dispatcher invokes EnsureWebKitInitialized() without adding
> Dispatcher as a RenderProcessObserver and "AppView" won't be installed.
> 
> I think the right fix is to move RegisterExtension() call from
> Dispatcher::Dispatcher() to Dispatcher::WebKitInitialized(), given that
> RegisterExtension() needs blink to be initialized beforehand. WDYT?

I'm really not very familiar with this code. You might want to talk to lazyboy@
or kalman@.

[bashi, 2015-07-09 23:24:17]

bashi@chromium.org changed reviewers:
+ kalman@chromium.org

[bashi, 2015-07-09 23:26:59]

On 2015/07/08 13:35:02, Paul Meyer wrote:
> On 2015/07/08 08:24:19, bashi1 wrote:
> > On 2015/06/19 13:21:14, Paul Meyer wrote:
> > > On 2015/06/19 02:02:24, bashi1 wrote:
> > > > On 2015/06/19 00:18:40, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
> > > > (JOB_FAILED,
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > > > 
> > > > Hmm, I got an exception when I ran extensions_browsertests
> > > > --gtest_filter='AppViewTest.TestAppViewGoodDataShouldSucceed'.
> > > > 
> > > > [20377:20377:0619/105152:454387819124:INFO:CONSOLE(55)] "Uncaught
> TypeError:
> > > > AppView is not a function", source:
> > > > chrome-extension://naaenhbhcdbpibkeenmhfiaghedbaepm/main.js (55)
> > > > 
> > > > It seems that "AppView" is installed by
> > > > extensions/renderer/resources/guest_view/app_view/app_view.js. I have no
> > idea
> > > > why calling EnsureWebKitInitialized() in
> > RenderThreadImpl::RegisterExtension()
> > > > make it unavailable.
> > > > 
> > > > paulmeyer@, do you have any idea? `git log` shows that you touched
> > app_view.js
> > > > several times.
> > > 
> > > I'm not sure exactly what is happening, but it looks like lots of things
are
> > > going wrong before it even gets to where it can't recognize AppView. What
is
> > > happening with AppView is likely that app_view.js was just never run,
since
> > that
> > > is where the AppView function is defined. Something must have gone wrong
> > before
> > > the dispatcher was able to run that file.
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> > 
> > Sorry for the late response. Took a close look and found the cause.
> > 
> > To make sure that "AppView" is installed, we need to add an
> > extensions::Dispatcher as a RenderProcessObserver via
> > RenderThreadImpl::AddObserver() *before* EnsureWebKitInitialized() is
called.
> > OTOH, extensions::Dispatcher calls RenderTHreadImpl::RegisterExtension() in
> its
> > constructor. If we call EnsureWebKitInitialized() in RegisterExtension(),
> > creating a Dispatcher invokes EnsureWebKitInitialized() without adding
> > Dispatcher as a RenderProcessObserver and "AppView" won't be installed.
> > 
> > I think the right fix is to move RegisterExtension() call from
> > Dispatcher::Dispatcher() to Dispatcher::WebKitInitialized(), given that
> > RegisterExtension() needs blink to be initialized beforehand. WDYT?
> 
> I'm really not very familiar with this code. You might want to talk to
lazyboy@
> or kalman@.

Added kalman@ as a reviewer. Patch set #3 added a workaround. PTAL?

(Please refer the description and comment #12 for the context)

[not at google - send to devlin, 2015-07-10 16:26:11]

I would have thought that you only need the changes in dispatcher.cc? Why change
the other files?

https://codereview.chromium.org/1182083006/diff/40001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/1182083006/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:202:
RenderThread::Get()->RegisterExtension(SafeBuiltins::CreateV8Extension());
Put this behind an |is_webkit_initialized_| flag?

[bashi, 2015-07-14 00:00:24]

Sorry for the delayed response.

What I want to achieve with this CL is to make sure Blink is initialized before
any Blink APIs are used. RenderThreadImpl::RegisterExtension() breaks the
restriction (see the description) so I added EnsureWebKitInitialized() in the
function. This changes when EnsureWebKitInitialized() is called and caused the
problem described in comment #10 and #12. We cannot call RegisterExtension() in
the dispatcher constructor because:
- RegisterExtension() needs Blink to get initialized.
- EnsureWebKitInitialized() needs Dispatcher to get constructed to install
SafeBuiltins extensions.

That's why I need to change these files.

https://codereview.chromium.org/1182083006/diff/40001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/1182083006/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:202:
RenderThread::Get()->RegisterExtension(SafeBuiltins::CreateV8Extension());
On 2015/07/10 16:26:11, kalman wrote:
> Put this behind an |is_webkit_initialized_| flag?

That won't install SafeBuiltins because |is_webkit_initialized_| is always false
when Dispatcher is constructed.

[not at google - send to devlin, 2015-07-14 23:31:54]

lgtm

https://codereview.chromium.org/1182083006/diff/40001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/1182083006/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:202:
RenderThread::Get()->RegisterExtension(SafeBuiltins::CreateV8Extension());
On 2015/07/14 00:00:24, bashi1 wrote:
> On 2015/07/10 16:26:11, kalman wrote:
> > Put this behind an |is_webkit_initialized_| flag?
> 
> That won't install SafeBuiltins because |is_webkit_initialized_| is always
false
> when Dispatcher is constructed.

Oh, right.

[bashi, 2015-07-14 23:48:22]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-07-14 23:48:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1182083006/#ps40001
(title: "Workaround for tests")

[commit-bot: I haz the power, 2015-07-14 23:49:28]

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

[commit-bot: I haz the power, 2015-07-14 23:57:55]

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

[commit-bot: I haz the power, 2015-07-14 23:57:56]

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

[bashi, 2015-07-15 00:00:29]

bashi@chromium.org changed reviewers:
+ kinuko@chromium.org

[bashi, 2015-07-15 00:02:50]

Kinuko-san, could you review changes in
chrome/renderer/chrome_content_renderer_client.cc ?

The change is just for testing. Please feel free to delegate other reviewers.

[kinuko, 2015-07-15 01:15:57]

lgtm

[bashi, 2015-07-15 01:22:23]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-07-15 01:23:08]

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

[commit-bot: I haz the power, 2015-07-15 01:30:55]

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

[commit-bot: I haz the power, 2015-07-15 01:30:57]

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

[bashi, 2015-07-15 01:35:18]

bashi@chromium.org changed reviewers:
+ jochen@chromium.org

[bashi, 2015-07-15 01:35:58]

On 2015/07/15 01:30:57, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

+jochen@ as chrome/renderer reviewers. PTAL?

[jochen (gone - plz use gerrit), 2015-07-15 13:36:42]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2015-07-15 13:36:43]

lgtm

[bashi, 2015-07-17 00:24:09]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-07-17 00:24:57]

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

[commit-bot: I haz the power, 2015-07-17 02:02:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-17 02:03:18]

Patchset 3 (id:??) landed as
https://crrev.com/0d244fa5e106b3d146655a2c623539a3e1900831
Cr-Commit-Position: refs/heads/master@{#339200}

[bashi, 2015-07-21 23:25:39]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1244243003/ by bashi@chromium.org.

The reason for reverting is: This seems the cause of many crashes.