Add signatures to FXJS_V8.

Add signatures to FXJS_V8.

BUG=chromium:529012
R=jochen@chromium.org, krasin@google.com

Committed: https://pdfium.googlesource.com/pdfium/+/158e335717efba9dce3aa6f6d1e31ed884e1f59e

[Tom Sepez, 2015-09-18 22:14:04]

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

[Tom Sepez, 2015-09-18 22:14:54]

Jochen, this is my best guess as how we would use signatures in PDFium, based
upon what I could piece together from the docs and the v8 test code.

[Tom Sepez, 2015-09-22 15:12:08]

On 2015/09/18 22:14:54, Tom Sepez wrote:
> Jochen, this is my best guess as how we would use signatures in PDFium, based
> upon what I could piece together from the docs and the v8 test code.
Ping?

[krasin, 2015-09-22 22:10:28]

krasin@google.com changed reviewers:
+ krasin@google.com

[krasin, 2015-09-22 22:10:28]

Hi jochen,

this CL resolves a hard blocker for Control Flow Integrity launch on Linux, see
http://crbug.com/529012. If possible, please, take a look.

[jochen (gone - plz use gerrit), 2015-09-25 11:30:19]

lgtm

https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c...
fpdfsdk/src/jsapi/fxjs_v8.cpp:21: static v8::Global<v8::ObjectTemplate>
g_DefaultGloabalObjectTemplate;
spelling of Global

[krasin, 2015-09-28 17:48:42]

rslgtm

[krasin, 2015-09-28 17:48:53]

lgtm

[Tom Sepez, 2015-09-28 18:13:38]

https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.c...
fpdfsdk/src/jsapi/fxjs_v8.cpp:21: static v8::Global<v8::ObjectTemplate>
g_DefaultGloabalObjectTemplate;
On 2015/09/25 11:30:19, jochen wrote:
> spelling of Global

Done.

[Tom Sepez, 2015-09-28 18:18:19]

Note to self: JS tests now failing, as if the check is over-zealous:

-Alert: PASS: this.removeField() threw error Document.removeField: Incorrect
number of parameters passed to function.
+Alert: PASS: this.removeField() threw error TypeError: Illegal invocation

[Tom Sepez, 2015-09-28 19:49:56]

PS#5 fixes:
> -Alert: PASS: this.removeField() threw error Document.removeField: Incorrect
> number of parameters passed to function.
> +Alert: PASS: this.removeField() threw error TypeError: Illegal invocation

But there's a segv when dealing with app. properties

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

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:70: v8::Local<v8::Signature> sig =
v8::Signature::New(pIsolate, fun);
you appear to put the functions on the prototype (below), but with a signature
requiring "fun". I'd try create the signature on  fun->PrototypeTemplate()

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:186: pObjDef->GetSignature()),
here ^^^

[Tom Sepez, 2015-09-29 15:50:34]

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:70: v8::Local<v8::Signature> sig =
v8::Signature::New(pIsolate, fun);
On 2015/09/29 07:30:47, jochen wrote:
> you appear to put the functions on the prototype (below), but with a signature
> requiring "fun". I'd try create the signature on  fun->PrototypeTemplate()

Hmm.  Signature ctor wants a function template, not an object template as would
be returned by PrototypeTemplate().

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

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:64:
fun->PrototypeTemplate()->SetInternalFieldCount(2);
this shouldn't be required (setting it on the instance template should be
enough)

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:70: v8::Local<v8::Signature> sig =
v8::Signature::New(pIsolate, fun);
On 2015/09/29 at 15:50:34, Tom Sepez wrote:
> On 2015/09/29 07:30:47, jochen wrote:
> > you appear to put the functions on the prototype (below), but with a
signature
> > requiring "fun". I'd try create the signature on  fun->PrototypeTemplate()
> 
> Hmm.  Signature ctor wants a function template, not an object template as
would be returned by PrototypeTemplate().

my bad, ignore this comment

https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:81: v8::Local<v8::ObjectTemplate>
GetPrototypeTemplate() {
this method should not be needed - replace all calls to GetPrototypeTemplate
with calls to GetInstanceTemplate

[Tom Sepez, 2015-09-30 17:17:25]

Ah, that does the trick. Jochen, please take a final look.  Thanks!

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

still lgtm

[Tom Sepez, 2015-09-30 22:40:02]

Committed patchset #6 (id:100001) manually as
158e335717efba9dce3aa6f6d1e31ed884e1f59e (presubmit successful).

[phoglund_chromium, 2015-10-01 12:16:42]

On 2015/09/30 22:40:02, Tom Sepez wrote:
> Committed patchset #6 (id:100001) manually as
> 158e335717efba9dce3aa6f6d1e31ed884e1f59e (presubmit successful).

This cl breaks the mac sizes steps, as it adds static initializers:
https://build.chromium.org/p/chromium/builders/Mac/builds/7707/steps/sizes/lo...
I reverted the pdfium roll now, please revert/fix this patch and then attempt to
roll again.

[phoglund_chromium, 2015-10-01 12:17:04]

phoglund@chromium.org changed reviewers:
+ phoglund@chromium.org

[Tom Sepez, 2015-10-01 15:38:14]

On 2015/10/01 12:17:05, phoglund_chrome wrote:

Argh, yes obviously, static v8:Local<> won't cut it.

[jochen (gone - plz use gerrit), 2015-10-01 15:40:04]

On 2015/10/01 at 15:38:14, tsepez wrote:
> On 2015/10/01 12:17:05, phoglund_chrome wrote:
> 
> Argh, yes obviously, static v8:Local<> won't cut it.

Maybe just put the Global into FXJSPerIsolateData?