Issue 5450 - introduce the Platform class

For present it's merely moving some platform specific interfaces from JsEngine as well as JsEngine itself into that Platform class.

[hub, 2017-07-28 14:29:03]

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:285: Platform& platform;
The problem I see here is that there is no guarantee that platform (a reference)
will never be dangling since there is no guarantee that the lifetime of platform
be longer than the JsEngine.

[sergei, 2017-07-28 15:10:29]

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:285: Platform& platform;
On 2017/07/28 14:29:03, hub wrote:
> The problem I see here is that there is no guarantee that platform (a
reference)
> will never be dangling since there is no guarantee that the lifetime of
platform
> be longer than the JsEngine.
> 

Right but it should be the responsibility of a user, the Platform class should
be destroyed only after releasing of all references to all modules (JsEngine,
FilterEngine, etc) from a user side. In order to better communicate it through
API I think we could return "JsEngine&" from `GetJsEngine` (a reference) instead
of a shared_ptr and maybe don't allow to freely construct JsEngine (only
Platform should be able to do it), but I think it should be done after moving of
FilterEngine into Platform.
Generally speaking the Platform class should become a parent class for its
modules. One of the reasons for it is that it's impossible to have a "self
waiting" class with internal asynchronous operations
(https://issues.adblockplus.org/ticket/5198) and pass a shared_ptr to this class
around, so either this Platform will wait for finishing of currently running
async operations or a user has to call some TearDown method. I think that it
would be better to put the call of such TearDown method into the destructor of
the Platform class, so a user should not remember about it and thus cannot
forget it. Yet one reasons is that actually there is already yet one similar
case (https://issues.adblockplus.org/ticket/4693) and that Platform class should
ensure that no async operation is running and safely destroy all modules.

BTW, the graceful shutting down seems requires to think a bit more
(https://issues.adblockplus.org/ticket/5118) because now I see that my comment
to #5118 is actually incorrect :).

[sergei, 2017-07-28 15:13:52]

On 2017/07/28 15:10:29, sergei wrote:
>
https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
> File include/AdblockPlus/JsEngine.h (right):
> 
>
https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
> include/AdblockPlus/JsEngine.h:285: Platform& platform;
> On 2017/07/28 14:29:03, hub wrote:
> > The problem I see here is that there is no guarantee that platform (a
> reference)
> > will never be dangling since there is no guarantee that the lifetime of
> platform
> > be longer than the JsEngine.
> > 
> 
> Right but it should be the responsibility of a user, the Platform class should
> be destroyed only after releasing of all references to all modules (JsEngine,
> FilterEngine, etc) from a user side. In order to better communicate it through
> API I think we could return "JsEngine&" from `GetJsEngine` (a reference)
instead
> of a shared_ptr and maybe don't allow to freely construct JsEngine (only
> Platform should be able to do it), but I think it should be done after moving
of
> FilterEngine into Platform.
Basically currently the fact that JsEngine accepts a reference to Platform
should already indicate that any reference to it should be released before
destroying of an instance of Platform.

[hub, 2017-07-28 18:16:38]

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:285: Platform& platform;
On 2017/07/28 15:10:29, sergei wrote:
> On 2017/07/28 14:29:03, hub wrote:
> > The problem I see here is that there is no guarantee that platform (a
> reference)
> > will never be dangling since there is no guarantee that the lifetime of
> platform
> > be longer than the JsEngine.
> > 
> 
> Right but it should be the responsibility of a user, the Platform class should
> be destroyed only after releasing of all references to all modules (JsEngine,
> FilterEngine, etc) from a user side. 

No. It is the responsibility of the API to make it sane and prevent that kind of
mistake.

> In order to better communicate it through
> API I think we could return "JsEngine&" from `GetJsEngine` (a reference)
instead
> of a shared_ptr and maybe don't allow to freely construct JsEngine (only
> Platform should be able to do it), but I think it should be done after moving
of
> FilterEngine into Platform.

This could help, yes, as it would keep the ownership of JsEngine into Platform.

[sergei, 2017-07-28 19:16:51]

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
File include/AdblockPlus/JsEngine.h (right):

https://codereview.adblockplus.org/29500602/diff/29500603/include/AdblockPlus...
include/AdblockPlus/JsEngine.h:285: Platform& platform;
On 2017/07/28 18:16:38, hub wrote:
> On 2017/07/28 15:10:29, sergei wrote:
> > On 2017/07/28 14:29:03, hub wrote:
> > > The problem I see here is that there is no guarantee that platform (a
> > reference)
> > > will never be dangling since there is no guarantee that the lifetime of
> > platform
> > > be longer than the JsEngine.
> > > 
> > 
> > Right but it should be the responsibility of a user, the Platform class
should
> > be destroyed only after releasing of all references to all modules
(JsEngine,
> > FilterEngine, etc) from a user side. 
> 
> No. It is the responsibility of the API to make it sane and prevent that kind
of
> mistake.
> 
> > In order to better communicate it through
> > API I think we could return "JsEngine&" from `GetJsEngine` (a reference)
> instead
> > of a shared_ptr and maybe don't allow to freely construct JsEngine (only
> > Platform should be able to do it), but I think it should be done after
moving
> of
> > FilterEngine into Platform.
> 
> This could help, yes, as it would keep the ownership of JsEngine into
Platform.
> 

But we cannot do it right now because FilterEngine is not moved yet and requires
a shared_ptr. I have added it into the issue description to don't forget about
it.

[hub, 2017-08-04 14:18:44]

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
File include/AdblockPlus/Platform.h (right):

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
include/AdblockPlus/Platform.h:100: std::shared_ptr<JsEngine> GetJsEngine();
I thought we had settled on returning a JsEngine& here.

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
include/AdblockPlus/Platform.h:129: std::shared_ptr<JsEngine> jsEngine;
...Which would make this a unique_ptr<JsEngine>

[sergei, 2017-08-04 14:40:09]

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
File include/AdblockPlus/Platform.h (right):

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
include/AdblockPlus/Platform.h:100: std::shared_ptr<JsEngine> GetJsEngine();
On 2017/08/04 14:18:43, hub wrote:
> I thought we had settled on returning a JsEngine& here.

We cannot do it right now because a lot of other code depends on it. I have
prepared changes firstly moving FilterEngine into Platform and then changing
this return value as well as a return value of GetFilterEngine to the
references. Meanwhile one can find them in
https://github.com/abby-sergz/libadblockplus/commits/ci-5450-references-api, I'm
going to send them to codereview after landing of this codereview.

BTW, there will still be a way to misuse it with JsValue:
JsValue jsValue = Plaltform().GetFilterEngine().GetPref("some pref");
or
JsValue jsValue = Plaltform().GetJsEngine().NewValue("some string");
jsValue.someMethod(); // here Platform with all internals is already destroyed.
I think we should simply say that it's not allowed to use anything after
destroying of Platform.

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
include/AdblockPlus/Platform.h:129: std::shared_ptr<JsEngine> jsEngine;
On 2017/08/04 14:18:43, hub wrote:
> ...Which would make this a unique_ptr<JsEngine>

This case is also a bit complicated, firstly we should get rid of
std::shared_ptr<JsEngine> everywhere in the code. I think it can be done later
in parallel.

BTW, having a shared pointer here can help to implement showing of a warning
when Platform is already destroyed but someone is still having a reference to
JsEngine, as we discussed somewhere.

[hub, 2017-08-04 15:14:50]

LGTM

But I would recommend to land the other patches (that needs review) all at the
same time

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
File include/AdblockPlus/Platform.h (right):

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
include/AdblockPlus/Platform.h:100: std::shared_ptr<JsEngine> GetJsEngine();
On 2017/08/04 14:40:09, sergei wrote:
> On 2017/08/04 14:18:43, hub wrote:
> > I thought we had settled on returning a JsEngine& here.
> 
> We cannot do it right now because a lot of other code depends on it. I have
> prepared changes firstly moving FilterEngine into Platform and then changing
> this return value as well as a return value of GetFilterEngine to the
> references. Meanwhile one can find them in
> https://github.com/abby-sergz/libadblockplus/commits/ci-5450-references-api,
I'm
> going to send them to codereview after landing of this codereview.
> 
> BTW, there will still be a way to misuse it with JsValue:
> JsValue jsValue = Plaltform().GetFilterEngine().GetPref("some pref");
> or
> JsValue jsValue = Plaltform().GetJsEngine().NewValue("some string");
> jsValue.someMethod(); // here Platform with all internals is already
destroyed.
> I think we should simply say that it's not allowed to use anything after
> destroying of Platform.

yes it is not bullet proof. Lifetime is not an easy problem, but just forbidding
it in the documentation isn't enough.

https://codereview.adblockplus.org/29500602/diff/29501577/include/AdblockPlus...
include/AdblockPlus/Platform.h:129: std::shared_ptr<JsEngine> jsEngine;
On 2017/08/04 14:40:09, sergei wrote:
> On 2017/08/04 14:18:43, hub wrote:
> > ...Which would make this a unique_ptr<JsEngine>
> 
> This case is also a bit complicated, firstly we should get rid of
> std::shared_ptr<JsEngine> everywhere in the code. I think it can be done later
> in parallel.
> 
> BTW, having a shared pointer here can help to implement showing of a warning
> when Platform is already destroyed but someone is still having a reference to
> JsEngine, as we discussed somewhere.

That suggestion was depending on the one above. Platform own the JsEngine.

[sergei, 2017-08-07 08:38:36]

On 2017/08/04 15:14:50, hub wrote:
> LGTM
> 
> But I would recommend to land the other patches (that needs review) all at the
> same time
I think it's possible to land them at the same time, but in different commits as
they are reviewed.