Opened 14 months ago

Last modified 9 months ago

#6526 new change

Stop using of deprecated V8 API.

Reported by: sergz Assignee: sergz
Priority: P3 Milestone:
Module: Libadblockplus Keywords:
Cc: rjeschke, hfiguiere Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29734562/
https://codereview.adblockplus.org/29735555/
https://codereview.adblockplus.org/29809555/
https://codereview.adblockplus.org/29810586/
https://codereview.adblockplus.org/29812646/
https://codereview.adblockplus.org/29812649/
https://codereview.adblockplus.org/29813591/
https://gitlab.com/eyeo/adblockplus/libadblockplus/merge_requests/7
https://gitlab.com/eyeo/adblockplus/libadblockplus/merge_requests/9

Description (last modified by sergz)

Background

We are using pretty a lot of deprecated V8 API, and in order to simplify our life, avoid unnecessary delays in such issues like #6063, and be able to experiment with new techniques like WebAssembly, we should update our code base to use up-to-date API.

What to change

The list of API

  • [done] v8::TryCatch should accept v8::Isolate
  • [done] v8::Isolate::IdleNotification is renamed to v8::Isolate::IdleNotitifcationDeadline
  • [done] use maybe version of v8::Script::Compile, related #6448
  • [done] use v8::Local instead of v8::Handle
  • [done] use maybe version of v8::Message::GetLineNumber
  • [question is below] use v8 functions which return maybe values, e.g.
    • v8::Value::IntegerValue
    • v8::FunctionTemplate::GetFunction
    • v8::String::NewFromUtf8
    • v8::Value::BooleanValue
    • v8::Object::Get(uint32_t)
    • v8::Object::GetOwnPropertyNames
    • v8::Function::Call
    • etc
  • those maybe functions accept v8::Context, therefore we need to carefully check that we pass the correct context. Perhaps in the callbacks we should rather use v8::Isolate::GetCurrentContext instead of obtaining it from JsEngine. Additionally pay attention to JsContext which works with the context too.
  • revisit ThrowExceptionInJS because failing throwing of an exception is not a good thing
  • add actual tests simulating the state of the isolate when it starts to return empty values.
  • to be continued ...

Here we need to decide regarding the behavior if there is no value in maybe value. The options are basically either to use an artificial default value, crash or propagate such resembling monadic approach to our API.

Change History (29)

comment:1 Changed 14 months ago by sergz

  • Review URL(s) modified (diff)

comment:2 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - update usage of V8 API

comment:3 Changed 14 months ago by sergz

  • Cc rjeschke hfiguiere added
  • Description modified (diff)
  • Review URL(s) modified (diff)

I would like to get your opinion especially regarding the dealing with the maybevalues in our code.

In order to get compiler warnings and errors one can add

  'defines': [
    'V8_DEPRECATION_WARNINGS',
    'V8_IMMINENT_DEPRECATION_WARNINGS'
  ],

to 'target_defaults' in common.gypi and compile our code with prebuilt V8.

comment:4 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - replace deprecated v8::Handle by v8::Local

comment:5 Changed 14 months ago by sergz

  • Description modified (diff)

comment:6 Changed 11 months ago by sergz

  • Owner set to sergz

comment:7 follow-up: Changed 11 months ago by hfiguiere

maybes are a good idea. As a Rust user I'm a big fan of Option<> (std::optional<> in C++ 17). While this often complicate code a bit, it definitely improve the code safety. It is like checking for nullptr.

I'm not sure if requiring C++17 for this is a good idea, but on the other hand we could have our own version of std::optional<> to implement this pattern.

comment:8 Changed 11 months ago by oleksandr

I also think propagating to our API is probably best in the long term. Having default values could cause unintended consequences and crashing just doesn't seem right to me.

comment:9 Changed 11 months ago by oleksandr

  • Description modified (diff)

comment:10 Changed 11 months ago by sergz

  • Review URL(s) modified (diff)

comment:11 in reply to: ↑ 7 Changed 11 months ago by sergz

Replying to hfiguiere:

I'm not sure if requiring C++17 for this is a good idea, but on the other hand we could have our own version of std::optional<> to implement this pattern.

Yeah, taking into account that it's compiled by android NDK and possibly other not so fast toolsets, C++17 can be a challenge. Own implementation of std::optional is also causing rather more questions so far.

I would propose to start with own implementation of std::optional, let's call it Maybe with the explicit template specification only for JsValue so far, since it seems can be implemented in a relatively efficient way. Perhaps it will also cover most cases of the deprecated API.
JIC, here I mean that there will be a special implementation to work with internals of JsValue, not a generic one.

Additionally what do you think about map/flatmap/bind/get/ifPresent/orElse/etc functions (of course add them only when they are required) either as a mixin (perhaps via CRTP) or just template functions? E.g. it seems we are not using thousands of operations with them, so any overhead like lambdas should not be an issue but perhaps it reduces a huge arising boilerplate code.

Last edited 11 months ago by sergz (previous) (diff)

comment:12 Changed 11 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:13 Changed 11 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:15 Changed 11 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:16 Changed 11 months ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - Use the maybe version of Compile() and Run()

comment:17 Changed 11 months ago by hfiguiere

  • Description modified (diff)

comment:18 Changed 11 months ago by hfiguiere

  • Description modified (diff)

comment:19 Changed 11 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:20 Changed 11 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:21 Changed 11 months ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - Remove deprecated CreateDefaultPlatform()

comment:22 Changed 11 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:23 Changed 10 months ago by hfiguiere

  • Description modified (diff)

comment:24 Changed 10 months ago by sergz

  • Description modified (diff)

fix layout/formating

comment:26 Changed 10 months ago by abpbot

comment:27 Changed 10 months ago by hfiguiere

  • Review URL(s) modified (diff)

comment:28 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 6526 - Fix test by not exposing V8

comment:29 Changed 9 months ago by sergz

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.