Don't initialize V8 via Blink in ppapi plugins

Don't initialize V8 via Blink in ppapi plugins

Instead, for the PDF plugin, use gin to manage V8.

Also add myself as owner for the v8 stuff.

BUG=530093
R=raymes@chromium.org,thestig@chromium.org

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

[jochen (gone - plz use gerrit), 2015-09-16 15:06:47]

the reason we can't use Blink's v8::Isolate is that Blink runs a bunch of
asserts on it that try to catch Blink API abuse. In debug builds, those asserts
trigger constantly for PDF files.

gin depends on base and v8 which are already both dependencies, so adding it
won't pull in a lot of new code.

[Lei Zhang, 2015-09-16 21:22:24]

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc
File pdf/pdf.cc (right):

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode33
pdf/pdf.cc:33: if (g_sdk_initialized_via_pepper) {
Didn't we just check |g_sdk_initialized_via_pepper| 2 lines up?

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode59
pdf/pdf.cc:59: g_isolate_holder =
CreateInstance() only gets called once per process, right? Otherwise we can leak
a previous |g_isolate_holder|.

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode63
pdf/pdf.cc:63: g_isolate_holder->isolate()->Exit();
Write a function for these 3 lines? Then reused on line 34.

[raymes, 2015-09-17 02:30:41]

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc
File pdf/pdf.cc (right):

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode59
pdf/pdf.cc:59: g_isolate_holder =
On 2015/09/16 21:22:24, Lei Zhang wrote:
> CreateInstance() only gets called once per process, right? Otherwise we can
leak
> a previous |g_isolate_holder|.

I believe CreateInstance will get called multiple times per process

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode61
pdf/pdf.cc:61: g_isolate_holder->isolate()->Enter();
nit: indentation looks funny here.

[jochen (gone - plz use gerrit), 2015-09-17 06:55:07]

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc
File pdf/pdf.cc (right):

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode33
pdf/pdf.cc:33: if (g_sdk_initialized_via_pepper) {
On 2015/09/16 at 21:22:24, Lei Zhang wrote:
> Didn't we just check |g_sdk_initialized_via_pepper| 2 lines up?

done

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode59
pdf/pdf.cc:59: g_isolate_holder =
On 2015/09/17 at 02:30:41, raymes wrote:
> On 2015/09/16 21:22:24, Lei Zhang wrote:
> > CreateInstance() only gets called once per process, right? Otherwise we can
leak
> > a previous |g_isolate_holder|.
> 
> I believe CreateInstance will get called multiple times per process

this block is protected by !g_sdk_initialized_via_pepper

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode61
pdf/pdf.cc:61: g_isolate_holder->isolate()->Enter();
On 2015/09/17 at 02:30:41, raymes wrote:
> nit: indentation looks funny here.

done

https://codereview.chromium.org/1346143003/diff/1/pdf/pdf.cc#newcode63
pdf/pdf.cc:63: g_isolate_holder->isolate()->Exit();
On 2015/09/16 at 21:22:24, Lei Zhang wrote:
> Write a function for these 3 lines? Then reused on line 34.

done

[Lei Zhang, 2015-09-17 07:09:36]

lgtm

[jochen (gone - plz use gerrit), 2015-09-17 07:20:30]

The CQ bit was checked by jochen@chromium.org

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

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

[commit-bot: I haz the power, 2015-09-17 08:13:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-17 08:14:39]

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

[raymes, 2015-09-18 01:57:03]

lgtm

[MikeM, 2015-10-29 22:15:58]

I think this patch is causing crashes in Debug when printing PDF's.
I'm trying to test out a patch to PDF support here:  
https://code.google.com/p/chromium/issues/detail?id=169120

It looks like V8 and current Isolate aren't setup yet (it should be) by the 
time we get the InitalizeSDK().
Call stack on crash is below:

     chrome_child.dll!v8::base::OS::Abort() Line 835    C++
     chrome_child.dll!v8::internal::Isolate::Current() Line 477    C++
     chrome_child.dll!chrome_pdf::InitializeSDK() Line 613    C++
     chrome_child.dll!chrome_pdf::GetPDFDocInfo(const void * pdf_buffer, 
int buffer_size, int * page_count, double * max_page_width) Line 144    C++
     chrome_child.dll!PrintingHandler::LoadPDF(base::File pdf_file) Line 
129    C++
     chrome_child.dll!PrintingHandler::OnRenderPDFPagesToMetafile(void * 
pdf_transit, const printing::PdfRenderSettings & settings) Line 76    C++
 
Can you help me on this?
Any suggestions on how to get past the crash?
Thanks,  Mike

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2015-10-30 05:39:39]

On 2015/10/29 22:15:58, mike.moening_gmail.com wrote:
> I think this patch is causing crashes in Debug when printing PDF's.
> I'm trying to test out a patch to PDF support here:  
> https://code.google.com/p/chromium/issues/detail?id=169120

Can you file a new bug and describe how triggered that crash? Certainly if we
crash while printing PDFs with unmodified Chromium, someone would have
noticed...

[Lei Zhang, 2015-10-30 05:40:05]

On 2015/10/30 05:39:39, Lei Zhang wrote:
> On 2015/10/29 22:15:58, http://mike.moening_gmail.com wrote:
> > I think this patch is causing crashes in Debug when printing PDF's.
> > I'm trying to test out a patch to PDF support here:  
> > https://code.google.com/p/chromium/issues/detail?id=169120
> 
> Can you file a new bug and describe how triggered that crash? Certainly if we
> crash while printing PDFs with unmodified Chromium, someone would have
> noticed...

... how *you* triggered ...

[MikeM, 2015-10-30 05:44:35]

Windows debug build multi- DLL should do it.
On Oct 30, 2015 12:40 AM, <thestig@chromium.org> wrote:

> On 2015/10/30 05:39:39, Lei Zhang wrote:
>
>> On 2015/10/29 22:15:58, http://mike.moening_gmail.com wrote:
>> > I think this patch is causing crashes in Debug when printing PDF's.
>> > I'm trying to test out a patch to PDF support here:
>> > https://code.google.com/p/chromium/issues/detail?id=169120
>>
>
> Can you file a new bug and describe how triggered that crash? Certainly if
>> we
>> crash while printing PDFs with unmodified Chromium, someone would have
>> noticed...
>>
>
> ... how *you* triggered ...
>
> https://codereview.chromium.org/1346143003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MikeM, 2015-10-30 05:47:33]

Chrome launches a utility process to do pdf printing. It crashes right
away. I filed a bug a few hours ago. Don't have # handy.
On Oct 30, 2015 12:44 AM, "Mike Moening" <mike.moening@gmail.com> wrote:

> Windows debug build multi- DLL should do it.
> On Oct 30, 2015 12:40 AM, <thestig@chromium.org> wrote:
>
>> On 2015/10/30 05:39:39, Lei Zhang wrote:
>>
>>> On 2015/10/29 22:15:58, http://mike.moening_gmail.com wrote:
>>> > I think this patch is causing crashes in Debug when printing PDF's.
>>> > I'm trying to test out a patch to PDF support here:
>>> > https://code.google.com/p/chromium/issues/detail?id=169120
>>>
>>
>> Can you file a new bug and describe how triggered that crash? Certainly
>>> if we
>>> crash while printing PDFs with unmodified Chromium, someone would have
>>> noticed...
>>>
>>
>> ... how *you* triggered ...
>>
>> https://codereview.chromium.org/1346143003/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2015-10-30 05:54:35]

On 2015/10/30 05:47:33, mike.moening_gmail.com wrote:
> Chrome launches a utility process to do pdf printing. It crashes right
> away. I filed a bug a few hours ago. Don't have # handy.

You can search for "reporter:me" on crbug.com. -> https://crbug.com/549365