Oilpan: Ensure that classes with virtual trace methods always have vtables for their left-most base…

Oilpan: Ensure that left-most vtable is the first thing to be initialized for polymorphic classes with trace methods.
    
This is done by ensuring one of two properties for any polymorphic class with a trace method.
1. If the trace method is virtual, then the left-most base class must define a virtual trace method too.
2. If the trace method is non-virtual, then the left-most base class must define some virtual method.
    
The Blink GC plugin statically checks these properties.

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

[zerny-chromium, 2014-08-11 14:52:03]

Plugin side change at: https://codereview.chromium.org/455263004/

PTAL

[haraken, 2014-08-11 15:15:26]

LGTM

[tkent, 2014-08-12 00:51:22]

lgtm

[haraken, 2014-08-12 00:55:10]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-12 00:56:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/463543002/1

[commit-bot: I haz the power, 2014-08-12 01:30:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19367)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22155)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40292)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/45537)

[commit-bot: I haz the power, 2014-08-12 01:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:37:33]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19372)

[Mads Ager (chromium), 2014-08-12 06:07:04]

LGTM with a couple of nits.

https://codereview.chromium.org/463543002/diff/1/Source/core/events/DOMWindow...
File Source/core/events/DOMWindowEventQueue.cpp (right):

https://codereview.chromium.org/463543002/diff/1/Source/core/events/DOMWindow...
Source/core/events/DOMWindowEventQueue.cpp:43: virtual void trace(Visitor*
visitor)
Separate the constructor and the trace method with a blank line?

https://codereview.chromium.org/463543002/diff/1/Source/core/fetch/ResourceCl...
File Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/463543002/diff/1/Source/core/fetch/ResourceCl...
Source/core/fetch/ResourceClient.h:28: #include "platform/heap/Handle.h"
You should be able to just forward declare Visitor (in the same way it is done
for Resource) instead of including Handle.h here.

https://codereview.chromium.org/463543002/diff/1/Source/platform/LifecycleCon...
File Source/platform/LifecycleContext.h (right):

https://codereview.chromium.org/463543002/diff/1/Source/platform/LifecycleCon...
Source/platform/LifecycleContext.h:33: #include "platform/heap/Handle.h"
Forward declare instead of include?

[haraken, 2014-08-12 06:15:51]

https://codereview.chromium.org/463543002/diff/1/Source/core/fetch/ResourceCl...
File Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/463543002/diff/1/Source/core/fetch/ResourceCl...
Source/core/fetch/ResourceClient.h:28: #include "platform/heap/Handle.h"
On 2014/08/12 06:07:04, Mads Ager (chromium) wrote:
> You should be able to just forward declare Visitor (in the same way it is done
> for Resource) instead of including Handle.h here.

I don't have a strong opinion here, but I guess that the current rule would be
just "include Handle.h so that the code compiles". Given that Handle.h has no
risk of creating include cycles, the rule will just work. (We're following the
rule for v8.h.)

[zerny-chromium, 2014-08-12 11:49:40]

The current requirements are too conservative and end up forcing us to make all
trace methods on FINAL-ScriptWrappable classes virtual which is simply not
needed. We have discussed a better set of requirements which I will update in
the plugin CL and then update this CL with a hopefully much smaller patch set.

[zerny-chromium, 2014-08-13 12:19:43]

Simplified. PTAL.

[haraken, 2014-08-13 12:25:34]

LGTM

[sof, 2014-08-13 12:29:09]

https://codereview.chromium.org/463543002/diff/20001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/463543002/diff/20001/Source/platform/Suppleme...
Source/platform/Supplementable.h:162: class SupplementableTracing<T, true> {
What value does this templated class add now?

[Mads Ager (chromium), 2014-08-13 12:32:16]

LGTM, with Sigbjorn's nit.

[zerny-chromium, 2014-08-13 13:07:02]

Thanks for the reviews!

https://codereview.chromium.org/463543002/diff/20001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/463543002/diff/20001/Source/platform/Suppleme...
Source/platform/Supplementable.h:162: class SupplementableTracing<T, true> {
On 2014/08/13 12:29:09, sof wrote:
> What value does this templated class add now?

None really. I'll remove it. The reason it is/was there is because
SupplementableBase should be a GC mixin in the oilpan case, but that causes too
many plugin-check complications that depended on oilpan_enabled. So we removed
it again.

[zerny-chromium, 2014-08-13 13:07:07]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:08:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/463543002/40001

[commit-bot: I haz the power, 2014-08-13 14:11:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22542)

[sof, 2014-08-13 14:28:02]

On 2014/08/13 13:07:02, zerny-chromium wrote:
> Thanks for the reviews!
> 
>
https://codereview.chromium.org/463543002/diff/20001/Source/platform/Suppleme...
> File Source/platform/Supplementable.h (right):
> 
>
https://codereview.chromium.org/463543002/diff/20001/Source/platform/Suppleme...
> Source/platform/Supplementable.h:162: class SupplementableTracing<T, true> {
> On 2014/08/13 12:29:09, sof wrote:
> > What value does this templated class add now?
> 
> None really. I'll remove it. The reason it is/was there is because
> SupplementableBase should be a GC mixin in the oilpan case, but that causes
too
> many plugin-check complications that depended on oilpan_enabled. So we removed
> it again.

lgtm

[commit-bot: I haz the power, 2014-08-13 15:27:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 15:27:58]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22552)

[zerny-chromium, 2014-08-14 06:51:21]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-14 06:51:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/463543002/40001

[commit-bot: I haz the power, 2014-08-14 06:52:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 06:52:09]

Failed to apply patch for Source/core/dom/ExecutionContext.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/ExecutionContext.cpp
  Hunk #1 FAILED at 328.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/dom/ExecutionContext.cpp.rej

Patch:       Source/core/dom/ExecutionContext.cpp
Index: Source/core/dom/ExecutionContext.cpp
diff --git a/Source/core/dom/ExecutionContext.cpp
b/Source/core/dom/ExecutionContext.cpp
index
115cad2495075ef2a787419b80509d34e13b0e91..34a629b3fae3353b5f74a790399d27708b448106
100644
--- a/Source/core/dom/ExecutionContext.cpp
+++ b/Source/core/dom/ExecutionContext.cpp
@@ -328,7 +328,8 @@ void ExecutionContext::trace(Visitor* visitor)
 #if ENABLE(OILPAN)
     visitor->trace(m_pendingExceptions);
 #endif
-    WillBeHeapSupplementable<blink::ExecutionContext>::trace(visitor);
+    WillBeHeapSupplementable<ExecutionContext>::trace(visitor);
+    LifecycleContext<ExecutionContext>::trace(visitor);
 }
 
 } // namespace blink

[zerny-chromium, 2014-08-14 07:00:56]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-14 07:01:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/463543002/60001

[commit-bot: I haz the power, 2014-08-14 10:44:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[commit-bot: I haz the power, 2014-08-14 13:11:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 13:11:36]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[zerny-chromium, 2014-08-14 14:10:45]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-14 14:11:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/463543002/60001

[commit-bot: I haz the power, 2014-08-14 15:27:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[commit-bot: I haz the power, 2014-08-14 16:33:18]

Committed patchset #4 (60001) as 180281