Bug 159153 – RunLoop::Timer should use constructor templates instead of class templates

• Component: New Bugs
• Status: RESOLVED FIXED

[Blaze Burg]

This should match the cleanup done to WebCore::Timer.

[Blaze Burg]

Created attachment 282149 [details]
Proposed Fix

[WebKit Commit Bot]

Attachment 282149 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RunLoop.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/RunLoop.h:149:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 83 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 282152 [details]
Proposed Fix

[WebKit Commit Bot]

Attachment 282152 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RunLoop.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/RunLoop.h:149:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 85 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 282154 [details]
Proposed Fix

[WebKit Commit Bot]

Attachment 282154 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RunLoop.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/RunLoop.h:149:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 282155 [details]
Proposed Fix (fixed changelog)

[WebKit Commit Bot]

Attachment 282155 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RunLoop.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/RunLoop.h:149:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Created attachment 282160 [details]
Proposed Fix (fix USE(SOUP))

[WebKit Commit Bot]

Attachment 282160 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RunLoop.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/RunLoop.h:149:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 282160 [details]
Proposed Fix (fix USE(SOUP))

View in context: https://bugs.webkit.org/attachment.cgi?id=282160&action=review

This feels nice.  r=me, don't break gtk.

> Source/WebCore/platform/network/ResourceHandleInternal.h:94
> -            , m_timeoutSource(RunLoop::main(), loader, &ResourceHandle::timeoutFired)
> +            , m_timeoutSource(RunLoop::main(), *loader, &ResourceHandle::timeoutFired)

loader should be a ResourceHandle&.

[Blaze Burg]

Created attachment 282261 [details]
Fix GTK

[WebKit Commit Bot]

Attachment 282261 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RunLoop.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/RunLoop.h:149:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Committed r202580: <http://trac.webkit.org/changeset/202580>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159245

[Blaze Burg]

<rdar://problem/27070856>

[Blaze Burg]

This was rolled out because it caused Production and Production GuardMalloc to fail. Investigating here.

[Blaze Burg]

Some relevant register state and disassembly from the point where it crashed under WKTR:

(lldb) reg read
General Purpose Registers:
       rax = 0x00000001011492c0  WebKit`vtable for WebKit::WebInspectorProxy + 64
       rbx = 0x000000010367e758
       rcx = 0x000000010116fb28  (void *)0x001d80010116fb51
       rdx = 0x001d80010116fb29
       rdi = 0x000000010367e78c
       rsi = 0x00000001001e25d0  JavaScriptCore`WTF::initializeMainThreadOnce()
       rbp = 0x00007fff5fbfe390
       rsp = 0x00007fff5fbfe370
        r8 = 0x00000000000000f8
        r9 = 0x000000010367e740
       r10 = 0x003a4601003a4680
       r11 = 0x000000010106ab22  WebKit`-[WKObject _apiObject]
       r12 = 0x0000000104401301
       r13 = 0x0000000000000000
       r14 = 0x000000010367e768
       r15 = 0x000000010a000018
       rip = 0x0000000100dd4641  WebKit`WebKit::WebInspectorProxy::WebInspectorProxy(WebKit::WebPageProxy*) + 57
    rflags = 0x0000000000010206
        cs = 0x000000000000002b
        fs = 0x0000000000000000
        gs = 0x0000000000000000

(lldb) di -f
WebKit`WebKit::WebInspectorProxy::WebInspectorProxy:
    0x100dd4608 <+0>:   pushq  %rbp
    0x100dd4609 <+1>:   movq   %rsp, %rbp
    0x100dd460c <+4>:   pushq  %r15
    0x100dd460e <+6>:   pushq  %r14
    0x100dd4610 <+8>:   pushq  %rbx
    0x100dd4611 <+9>:   pushq  %rax
    0x100dd4612 <+10>:  movq   %rsi, %r15
    0x100dd4615 <+13>:  movq   %rdi, %rbx
    0x100dd4618 <+16>:  callq  0x100e4b874               ; API::Object::Object()
    0x100dd461d <+21>:  leaq   0x10(%rbx), %r14
    0x100dd4621 <+25>:  leaq   0x374c68(%rip), %rax      ; vtable for WebKit::WebInspectorProxy + 16
    0x100dd4628 <+32>:  movq   %rax, (%rbx)
    0x100dd462b <+35>:  leaq   0x374c8e(%rip), %rax      ; vtable for WebKit::WebInspectorProxy + 64
    0x100dd4632 <+42>:  movq   %rax, 0x10(%rbx)
    0x100dd4636 <+46>:  movq   %r15, 0x18(%rbx)
    0x100dd463a <+50>:  leaq   0x34(%rbx), %rdi
    0x100dd463e <+54>:  xorps  %xmm0, %xmm0
->  0x100dd4641 <+57>:  movaps %xmm0, 0x20(%rbx)
    0x100dd4645 <+61>:  movb   $0x0, 0x30(%rbx)
    0x100dd4649 <+65>:  callq  0x100e57d04               ; IPC::Attachment::Attachment()
    0x100dd464e <+70>:  movl   $0x0, 0x40(%rbx)
    0x100dd4655 <+77>:  movq   %rbx, %r15
    0x100dd4658 <+80>:  subq   $-0x80, %r15
    0x100dd465c <+84>:  movq   $0x0, 0x70(%rbx)
    0x100dd4664 <+92>:  movq   $0x0, 0x68(%rbx)
    0x100dd466c <+100>: movq   $0x0, 0x60(%rbx)
    0x100dd4674 <+108>: movq   $0x0, 0x58(%rbx)
    0x100dd467c <+116>: movq   $0x0, 0x50(%rbx)
    0x100dd4684 <+124>: movq   $0x0, 0x48(%rbx)
    0x100dd468c <+132>: callq  0x10109f44c               ; symbol stub for: WTF::RunLoop::main()
    0x100dd4691 <+137>: movq   %r15, %rdi
    0x100dd4694 <+140>: movq   %rax, %rsi
    0x100dd4697 <+143>: callq  0x10109f476               ; symbol stub for: WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)
    0x100dd469c <+148>: leaq   0x39c8ad(%rip), %rax      ; vtable for WTF::RunLoop::Timer + 16
    0x100dd46a3 <+155>: movq   %rax, 0x80(%rbx)
    0x100dd46aa <+162>: leaq   0xa0(%rbx), %rax
    0x100dd46b1 <+169>: movq   %rax, 0xc0(%rbx)
    0x100dd46b8 <+176>: leaq   0x39dee9(%rip), %rax      ; vtable for std::__1::__function::__func<std::__1::__bind<void (WebKit::WebInspectorProxy::*&)(), WebKit::WebInspectorProxy*>, std::__1::allocator<std::__1::__bind<void (WebKit::WebInspectorProxy::*&)(), WebKit::WebInspectorProxy*> >, void ()> + 16
    0x100dd46bf <+183>: movq   %rax, 0xa0(%rbx)
    0x100dd46c6 <+190>: movq   0x364d22(%rip), %xmm0     ; (void *)0x0000000100f917da: WebKit::WebInspectorProxy::closeTimerFired(), xmm0 = mem[0],zero 
    0x100dd46ce <+198>: movdqu %xmm0, 0xa8(%rbx)
    0x100dd46d6 <+206>: movq   %rbx, 0xb8(%rbx)
    0x100dd46dd <+213>: movq   $0x0, 0xd0(%rbx)
    0x100dd46e8 <+224>: movq   0x18(%rbx), %rax
    0x100dd46ec <+228>: movq   0xd0(%rax), %rdi
    0x100dd46f3 <+235>: movq   0x5b0(%rax), %rcx
    0x100dd46fa <+242>: leaq   0x33ae6c(%rip), %rsi      ; "WebInspectorProxy"
    0x100dd4701 <+249>: movl   $0x11, %edx
    0x100dd4706 <+254>: movq   %r14, %r8
    0x100dd4709 <+257>: addq   $0x8, %rsp
    0x100dd470d <+261>: popq   %rbx
    0x100dd470e <+262>: popq   %r14
    0x100dd4710 <+264>: popq   %r15
    0x100dd4712 <+266>: popq   %rbp
    0x100dd4713 <+267>: jmp    0x100e64cd0               ; WebKit::ChildProcessProxy::addMessageReceiver(IPC::StringReference, unsigned long long, IPC::MessageReceiver&)

[Mark Lam]

Let’s dive into the assembly a bit and see what’s happening here:

WebKit`WebKit::WebInspectorProxy::WebInspectorProxy:
    0x100dd4608 <+0>:   pushq  %rbp                      // Save the caller frame pointer
    0x100dd4609 <+1>:   movq   %rsp, %rbp                // Setup the new frame pointer
    0x100dd460c <+4>:   pushq  %r15                      // Save a few registers r15, r14, rbx, rax.
    0x100dd460e <+6>:   pushq  %r14
    0x100dd4610 <+8>:   pushq  %rbx
    0x100dd4611 <+9>:   pushq  %rax
    0x100dd4612 <+10>:  movq   %rsi, %r15                // Copy arg1 (i.e. inspectedPage) to %r15.  %r15 is a callee saved reg.
    0x100dd4615 <+13>:  movq   %rdi, %rbx                // Copy arg0 (i.e. this pointer) to $rbx.  $rbx is a callee saved reg.
    0x100dd4618 <+16>:  callq  0x100e4b874               ; API::Object::Object() // Call the super’s super’s constructor i.e. WebKit2’s API::Object().

    0x100dd461d <+21>:  leaq   0x10(%rbx), %r14          // Set %r14 (a callee saved reg) = value at offset 0x10 ie.. 16 in this.
                                                         // Offset 0 is the vtbl pointer.
                                                         // Offset 8 is m_refCount from the inheritted ThreadSafeRefCountedBase.
                                                         // Offset 16 is NSObject* m_wrapper from API::Object.
                                                         // Hence, %r14 is this.m_wrapper.

    // Note: API::Object does not appear to initialize m_wrapper though.  So this looks funky.  Why is it loaded?

    0x100dd4621 <+25>:  leaq   0x374c68(%rip), %rax      ; vtable for WebKit::WebInspectorProxy + 16
                                                         // Set %rax = &vtbl 
    0x100dd4628 <+32>:  movq   %rax, (%rbx)              // Set this.vtbl = vtbl i.e install the WebInspectorProxy vtbl for API::ObjectImpl ???
    0x100dd462b <+35>:  leaq   0x374c8e(%rip), %rax      ; vtable for WebKit::WebInspectorProxy + 64
                                                         // Neat … another vtbl because WebInspectorProxy multiply inherits from IPC::MessageReceiver as well
                                                         // and IPC::MessageReceiver has virtual functions too.

    0x100dd4632 <+42>:  movq   %rax, 0x10(%rbx)          // What is this??? Why is this.m_wrapper being set to the 2nd vtbl?

    0x100dd4636 <+46>:  movq   %r15, 0x18(%rbx).         // Set this + offset 24 i.e. this.m_inspectedPage to %r15 i.e. inspectedPage.

    // We just initialized the m_inspectedPage field above.  The next step (according to the code for WebInspectorProxy’s constructor) is to call RunLoop::main().

    0x100dd463a <+50>:  leaq   0x34(%rbx), %rdi          // Set %rdi i.e. arg0 = &this + offset 0x34 i.e. 52

        // Strange!!!
        //      offset 0x18: m_inspectedPage
        //      offset 0x20: m_inspectorPage
        //      offset 0x28: m_underTest
        //      offset 0x29: m_isVisible
        //      offset 0x2a: m_isAttached
        //      offset 0x2b: m_canAttach
        //      offset 0x2c: m_isProfilingPage
        //      offset 0x2d: m_showMessageSent
        //      offset 0x2e: m_ignoreFirstBringToFront
        //      offset 0x2f: m_elementSelectionActive
        //      offset 0x30: m_ignoreElementSelectionChange

        // And presumably:
        //      offset 0x38: m_connectionIdentifier

        // What is at offset 0x34?

    0x100dd463e <+54>:  xorps  %xmm0, %xmm0              // xor xmm0 with self i.e. set xmm0 = 0.
->  0x100dd4641 <+57>:  movaps %xmm0, 0x20(%rbx)         // mov 0 to address %rbx + 0x20 i.e. this.m_inspectorPage.

        // Another bit of strangeness: why is instruction <+57>:  crashing?  The only memory we access here is %rbx, and we’re just trying to set this.m_inspectorPage to nullptr.
        // Note that we had just access the written to the contents of this object just a few instructions up e.g. <+46>:, <+42>:, and <+32>:, and those did not result in a crash.

I did my analysis above based on the source from ToT.  Maybe some code has changed, and this disassembly doesn't match what's in ToT.  Or maybe I made a mistake somewhere.

Brian, can you please recheck my analysis with the proper source code?

[Kimmo Kinnunen]

Pull request: https://github.com/WebKit/WebKit/pull/7693

[EWS]

Committed 257984@main (d94e96e0fc4a): <https://commits.webkit.org/257984@main>

Reviewed commits have been landed. Closing PR #7693 and removing active labels.