Componentization: add toV8(void*,...) to check function overload mismatch.

Componentization: add toV8(void*,...) to check function overload mismatch.

Added toV8(void*,...) without any definition to find function overload
mismatch.
The toV8(void*,...) should be never used. So if we see any unresolved
symbols: toV8(void*,...), we will know that we have bugs.
If we don't have toV8(void*,...), such buggy code will use toV8(bool,...).
This makes it a little difficult for us to find the bugs.

BUG=358074
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194789

[tasak, 2015-04-30 07:02:54]

tasak@google.com changed reviewers:
+ haraken@chromium.org

[tasak, 2015-04-30 07:02:55]

PTAL?

https://codereview.chromium.org/1118653002/diff/1/Source/modules/background_s...
File Source/modules/background_sync/SyncCallbacks.h (right):

https://codereview.chromium.org/1118653002/diff/1/Source/modules/background_s...
Source/modules/background_sync/SyncCallbacks.h:8: #include
"core/dom/DOMException.h"
modules.SyncCallbacks.obj : error LNK2019: unresolved external symbol "class
v8::Local<class v8::Value> __cdecl blink::toV8(void *,class v8::Local<class
v8::Object>,class v8::Isolate *)"

https://codereview.chromium.org/1118653002/diff/1/Source/modules/push_messagi...
File Source/modules/push_messaging/PushSubscription.h (right):

https://codereview.chromium.org/1118653002/diff/1/Source/modules/push_messagi...
Source/modules/push_messaging/PushSubscription.h:11: #include
"core/dom/DOMException.h"
modules.PushSubscription.obj : error LNK2001: unresolved external symbol "class
v8::Local<class v8::Value> __cdecl blink::toV8(void *,class v8::Local<class
v8::Object>,class v8::Isolate *)"

[haraken, 2015-04-30 07:15:48]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2015-04-30 07:15:48]

shiino-san: Would you mind taking a look at this?

[Jens Widell, 2015-04-30 11:20:31]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-04-30 11:20:32]

Would declaring

  template<typename T>
  v8::Handle<v8::Value> toV8(T* value, [...]);

have the same effect, but with a (slightly nicer?) error message that contains T
rather than void*?

Adding (either) overload to catch errors is a very good idea, I think, so that
LGTM.

I don't really understand why including DOMException.h in some header files was
necessary, though. What in those header files depended on having DOMException.h
included?

[tasak, 2015-04-30 12:12:10]

Thank you for review.

On 2015/04/30 11:20:32, Jens Widell wrote:
> Would declaring
> 
>   template<typename T>
>   v8::Handle<v8::Value> toV8(T* value, [...]);
> 
> have the same effect, but with a (slightly nicer?) error message that contains
T
> rather than void*?

I think, we need more complicated templates for toV8, because if the class T
derives from ScriptWrappable, we should use toV8(ScriptWrappable*,...).
I guess, we need to use WTF::IsSubclassOf or something.

The original problem is that some code uses toV8(bool, ...) instead of
toV8(ScriptWrappable*,...) when using separate build.

> Adding (either) overload to catch errors is a very good idea, I think, so that
> LGTM.
> 
> I don't really understand why including DOMException.h in some header files
was
> necessary, though. What in those header files depended on having
DOMException.h
> included?

I found those header files by using linker errors, i.e.

modules.SyncCallbacks.obj : error LNK2019: unresolved external symbol "class
v8::Local<class v8::Value> __cdecl blink::toV8(void *,class v8::Local<class
v8::Object>,class v8::Isolate *)"
(?toV8@blink@@YA?AV?$Local@VValue@v8@@@v8@@PEAXV?$Local@VObject@v8@@@3@PEAVIsolate@3@@Z)
referenced in function "private: void __cdecl
blink::ScriptPromiseResolver::resolveOrReject<class blink::DOMException *>(class
blink::DOMException *,enum blink::ScriptPromiseResolver::ResolutionState)"
(??$resolveOrReject@PEAVDOMException@blink@@@ScriptPromiseResolver@blink@@AEAAXPEAVDOMException@1@W4ResolutionState@01@@Z)
modules.PushSubscription.obj : error LNK2001: unresolved external symbol "class
v8::Local<class v8::Value> __cdecl blink::toV8(void *,class v8::Local<class
v8::Object>,class v8::Isolate *)"
(?toV8@blink@@YA?AV?$Local@VValue@v8@@@v8@@PEAXV?$Local@VObject@v8@@@3@PEAVIsolate@3@@Z)
modules.PushSubscriptionCallbacks.obj : error LNK2001: unresolved external
symbol "class v8::Local<class v8::Value> __cdecl blink::toV8(void *,class
v8::Local<class v8::Object>,class v8::Isolate *)"
(?toV8@blink@@YA?AV?$Local@VValue@v8@@@v8@@PEAXV?$Local@VObject@v8@@@3@PEAVIsolate@3@@Z)

So looking at the signatures, I found that template<typename T> resolveOrReject
in ScriptPromiseResolver needs T's definition, and the unknown class was
DOMException.

If possible, I would like to find such mismatches during compiling time instead
of during linking time.

[Jens Widell, 2015-04-30 12:38:41]

On 2015/04/30 12:12:10, tasak wrote:
> Thank you for review.
> 
> On 2015/04/30 11:20:32, Jens Widell wrote:
> > Would declaring
> > 
> >   template<typename T>
> >   v8::Handle<v8::Value> toV8(T* value, [...]);
> > 
> > have the same effect, but with a (slightly nicer?) error message that
contains
> T
> > rather than void*?
> 
> I think, we need more complicated templates for toV8, because if the class T
> derives from ScriptWrappable, we should use toV8(ScriptWrappable*,...).
> I guess, we need to use WTF::IsSubclassOf or something.

Ah, right, that's true. A simple void* solution is probably to prefer in that
case, I think.

> The original problem is that some code uses toV8(bool, ...) instead of
> toV8(ScriptWrappable*,...) when using separate build.
> 
> > Adding (either) overload to catch errors is a very good idea, I think, so
that
> > LGTM.
> > 
> > I don't really understand why including DOMException.h in some header files
> was
> > necessary, though. What in those header files depended on having
> DOMException.h
> > included?
> 
> I found those header files by using linker errors, i.e.
> 
> modules.SyncCallbacks.obj : error LNK2019: unresolved external symbol "class
> v8::Local<class v8::Value> __cdecl blink::toV8(void *,class v8::Local<class
> v8::Object>,class v8::Isolate *)"
>
(?toV8@blink@@YA?AV?$Local@VValue@v8@@@v8@@PEAXV?$Local@VObject@v8@@@3@PEAVIsolate@3@@Z)
> referenced in function "private: void __cdecl
> blink::ScriptPromiseResolver::resolveOrReject<class blink::DOMException
*>(class
> blink::DOMException *,enum blink::ScriptPromiseResolver::ResolutionState)"
>
(??$resolveOrReject@PEAVDOMException@blink@@@ScriptPromiseResolver@blink@@AEAAXPEAVDOMException@1@W4ResolutionState@01@@Z)
> modules.PushSubscription.obj : error LNK2001: unresolved external symbol
"class
> v8::Local<class v8::Value> __cdecl blink::toV8(void *,class v8::Local<class
> v8::Object>,class v8::Isolate *)"
>
(?toV8@blink@@YA?AV?$Local@VValue@v8@@@v8@@PEAXV?$Local@VObject@v8@@@3@PEAVIsolate@3@@Z)
> modules.PushSubscriptionCallbacks.obj : error LNK2001: unresolved external
> symbol "class v8::Local<class v8::Value> __cdecl blink::toV8(void *,class
> v8::Local<class v8::Object>,class v8::Isolate *)"
>
(?toV8@blink@@YA?AV?$Local@VValue@v8@@@v8@@PEAXV?$Local@VObject@v8@@@3@PEAVIsolate@3@@Z)
> 
> So looking at the signatures, I found that template<typename T>
resolveOrReject
> in ScriptPromiseResolver needs T's definition, and the unknown class was
> DOMException.

At least in SyncCallbacks.h, ScriptPromiseResolver looks to be just
forward-declared, so how would the compiler know its resolveOrReject() method
uses toV8?

> If possible, I would like to find such mismatches during compiling time
instead
> of during linking time.

What happens if you have

  void toV8(void*, v8::Local<v8::Object>, v8::Isolate*);

instead? Presumably, the compiler should still pick this overload, and then
hopefully run into compile-time trouble because of the void return value.

I can't seem to trigger any of your errors at all by adding any toV8(void*,
...), so I can't test it. Might be because I'm building on Linux.

[tasak, 2015-05-01 02:12:23]

On 2015/04/30 12:38:41, Jens Widell wrote:

> What happens if you have
> 
>   void toV8(void*, v8::Local<v8::Object>, v8::Isolate*);
> 
> instead?

I would like to add static_assert() to the template code for T="void*".

So I think, clang or msvc shows how the template matches...
I expect it would be a little easier to know where to add some header file.

I also thinking of using __PRETTY_FUNCTION__ (for clang) or __FUNCSIG__ (for
msvc).
These are useful for debugging templates.

> I can't seem to trigger any of your errors at all by adding any toV8(void*,
> ...), so I can't test it. Might be because I'm building on Linux.

The error occurs when component=shared_library and
link_core_modules_separately=1.

Unfortunately, my inspector patch has not been committed yet.
I tested the code locally.

[Yuki, 2015-05-01 04:26:42]

lgtm

https://codereview.chromium.org/1118653002/diff/1/Source/bindings/core/v8/ToV8.h
File Source/bindings/core/v8/ToV8.h (right):

https://codereview.chromium.org/1118653002/diff/1/Source/bindings/core/v8/ToV...
Source/bindings/core/v8/ToV8.h:242: // because of "unresolved external symbol".
Could you add a following comment?

Without toV8(void*, ...), call to toV8 with T* will match with toV8(bool, ...)
if T is not a subclass of ScriptWrappable or if T is declared but not defined
(so it's not clear that T is a subclass of ScriptWrappable).  This hack helps
detect such unwanted implicit conversions from T* to bool.

[tasak, 2015-05-01 05:16:50]

Thank you for review.

haraken@, I moved #include "core/dom/DOMException.h" to SyncError.h and
PushError.h.

https://codereview.chromium.org/1118653002/diff/1/Source/bindings/core/v8/ToV8.h
File Source/bindings/core/v8/ToV8.h (right):

https://codereview.chromium.org/1118653002/diff/1/Source/bindings/core/v8/ToV...
Source/bindings/core/v8/ToV8.h:242: // because of "unresolved external symbol".
On 2015/05/01 04:26:42, Yuki (ooo til may 11) wrote:
> Could you add a following comment?
> 
> Without toV8(void*, ...), call to toV8 with T* will match with toV8(bool, ...)
> if T is not a subclass of ScriptWrappable or if T is declared but not defined
> (so it's not clear that T is a subclass of ScriptWrappable).  This hack helps
> detect such unwanted implicit conversions from T* to bool.

Done.

[haraken, 2015-05-01 05:34:57]

LGTM

[tasak, 2015-05-01 05:44:55]

The CQ bit was checked by tasak@google.com

[tasak, 2015-05-01 05:44:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jl@opera.com,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1118653002/#ps40001
(title: "Added comment")

[commit-bot: I haz the power, 2015-05-01 05:45:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1118653002/40001

[commit-bot: I haz the power, 2015-05-01 06:39:08]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194789