Don`t use references to JsEngine to avoid use-after-free errors,switch to shared_ptr instead

Don`t use references to JsEngine to avoid use-after-free errors,
switch to shared_ptr instead

[Felix Dahlke, 2013-04-18 18:42:16]

http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp#newcode54
src/JsEngine.cpp:54: AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const
AppInfo& appInfo)
I don't think we should have this. "JsEnginePtr x(new JsEngine(...))" is
actually shorter than "JsEnginePtr x(JsEngine::New(...))", and it's idiomatic
too.

http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp#newcode58
src/JsEngine.cpp:58: const v8::Locker locker(result->isolate);
This is initialisation code, can you move it to the constructor?

[Wladimir Palant, 2013-04-18 20:24:28]

http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp
File src/JsEngine.cpp (right):

http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp#newcode54
src/JsEngine.cpp:54: AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const
AppInfo& appInfo)
On 2013/04/18 18:42:16, Felix H. Dahlke wrote:
> I don't think we should have this. "JsEnginePtr x(new JsEngine(...))" is
> actually shorter than "JsEnginePtr x(JsEngine::New(...))", and it's idiomatic
> too.

We need to have the object in a shared_ptr before we can use it properly -
shared_from_this() will not work in a constructor and we won't be able to pass
anything to context setup functions. Hence the separation between object
constructor and initialization.

[Felix Dahlke, 2013-04-19 02:33:29]

On 2013/04/18 20:24:28, Wladimir Palant wrote:
> http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp
> File src/JsEngine.cpp (right):
> 
> http://codereview.adblockplus.org/10173031/diff/1/src/JsEngine.cpp#newcode54
> src/JsEngine.cpp:54: AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const
> AppInfo& appInfo)
> On 2013/04/18 18:42:16, Felix H. Dahlke wrote:
> > I don't think we should have this. "JsEnginePtr x(new JsEngine(...))" is
> > actually shorter than "JsEnginePtr x(JsEngine::New(...))", and it's
idiomatic
> > too.
> 
> We need to have the object in a shared_ptr before we can use it properly -
> shared_from_this() will not work in a constructor and we won't be able to pass
> anything to context setup functions. Hence the separation between object
> constructor and initialization.

Hm, I see. Unfortunate but I guess it can't be helped...

LGTM