Bug 152482 - [WK2] Looping in testcase: fast/dom/Window/property-access-on-cached-window-after-frame-removed.html
Summary: [WK2] Looping in testcase: fast/dom/Window/property-access-on-cached-window-a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-21 10:49 PST by Jiewen Tan
Modified: 2015-12-22 09:38 PST (History)
6 users (show)

See Also:


Attachments
Test Case (944 bytes, text/html)
2015-12-21 17:05 PST, Jiewen Tan
no flags Details
Test Case Resource (4.25 KB, text/javascript)
2015-12-21 17:06 PST, Jiewen Tan
no flags Details
patch (3.08 KB, patch)
2015-12-21 23:19 PST, chris fleizach
bfulgham: review+
bfulgham: commit-queue+
Details | Formatted Diff | Diff
patch (4.22 KB, patch)
2015-12-21 23:30 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2015-12-21 10:49:13 PST
WK2 will keep looping in fast/dom/Window/property-access-on-cached-window-after-frame-removed.html while WK1 won't.
Investigate why it keeps looping.
Comment 1 Jiewen Tan 2015-12-21 14:36:00 PST
The looping path is:
accessibilityController,rootElement,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,verticalScrollbar,endTextMarker
Comment 2 Jiewen Tan 2015-12-21 17:04:33 PST
The looping can be explained as follow:
The testcase is trying to do a DFS of window object's properties. Since verticalScrollbar itself is an AcessibilityUIElement, and AcessibilityUIElement class by default has verticalScrollbar property, DFS will be stuck.

Actually, the class abstract is the same for both WebkitTestRunner and DumpRenderTree for AcessibilityUIElement. The difference is how they implement it.

In WebkitTestRunner:
PassRefPtr<AccessibilityUIElement> AccessibilityUIElement::verticalScrollbar() const
{
    BEGIN_AX_OBJC_EXCEPTIONS
    return AccessibilityUIElement::create([m_element accessibilityAttributeValue:NSAccessibilityVerticalScrollBarAttribute]);
    END_AX_OBJC_EXCEPTIONS        

    return nullptr;
}

PassRefPtr<AccessibilityUIElement> AccessibilityUIElement::create(PlatformUIElement uiElement)
{
    return adoptRef(new AccessibilityUIElement(uiElement));
}
Therefore, every time when the property is accessed, an new AccessibilityUIElement is created and returned. I am not sure which constructor it calls. I am assuming the follows:
AccessibilityUIElement::AccessibilityUIElement(PlatformUIElement element)
    : m_element(element)
    , m_notificationHandler(0)
{
    // FIXME: ap@webkit.org says ObjC objects need to be CFRetained/CFRelease to be GC-compliant on the mac.
    [m_element retain];
}

In DumpRenderTree:
AccessibilityUIElement AccessibilityUIElement::verticalScrollbar() const
{
    BEGIN_AX_OBJC_EXCEPTIONS
    return AccessibilityUIElement([m_element accessibilityAttributeValue:NSAccessibilityVerticalScrollBarAttribute]);
    END_AX_OBJC_EXCEPTIONS        

    return nullptr;
}

AccessibilityUIElement::AccessibilityUIElement(PlatformUIElement element)
    : m_element(element)
    , m_notificationHandler(0)
{
    // FIXME: ap@webkit.org says ObjC objects need to be CFRetained/CFRelease to be GC-compliant on the mac.
    [m_element retain];
}
Here, a ref will be returned directly.

In fact, the DumpRenderTree will loop as well, but it stops after the second try. Therefore, I am not sure whether it is due to some reasons specific to the language itself cause the problem. I suggest to handover the bug to the accessibility guy.
Comment 3 Jiewen Tan 2015-12-21 17:05:51 PST
Created attachment 267765 [details]
Test Case
Comment 4 Jiewen Tan 2015-12-21 17:06:10 PST
Created attachment 267766 [details]
Test Case Resource
Comment 5 chris fleizach 2015-12-21 23:19:01 PST
Created attachment 267781 [details]
patch
Comment 6 Jiewen Tan 2015-12-21 23:24:51 PST
Comment on attachment 267781 [details]
patch

Could you please remove the skip statement for the test case under LayoutTests/platform/mac-wk2/TestExpectations  L344 as well?
Comment 7 chris fleizach 2015-12-21 23:30:30 PST
Created attachment 267782 [details]
patch
Comment 8 Brent Fulgham 2015-12-22 08:49:58 PST
Comment on attachment 267781 [details]
patch

Thanks for fixing this so quickly! r=me.
Comment 9 WebKit Commit Bot 2015-12-22 09:38:24 PST
Comment on attachment 267782 [details]
patch

Clearing flags on attachment: 267782

Committed r194364: <http://trac.webkit.org/changeset/194364>
Comment 10 WebKit Commit Bot 2015-12-22 09:38:28 PST
All reviewed patches have been landed.  Closing bug.