Bug 104832 – [BlackBerry] Ensure InRegionScrollableArea is valid before using.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Mike Lattanzio]

Make sure we still have a RefPtr to pointer before using it. 
Otherwise it might have been deleted.

[Mike Lattanzio]

Created attachment 179099 [details]
Patch

[Antonio Gomes]

Comment on attachment 179099 [details]
Patch

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

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:420
> +bool InRegionScrollerPrivate::isValidScrollableLayerWebKitThread(WebCore::LayerWebKitThread* layerWebKitThread)

const

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:432
> +bool InRegionScrollerPrivate::isValidScrollableNode(WebCore::Node* node)

const

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:437
> +    for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++)

IIRC calculateActiveAndShrinkCachedScrollableAreas shrink this vector. Does it need to be a for loop?

[Mike Lattanzio]

(In reply to comment #2)
> (From update of attachment 179099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179099&action=review
> 
> > Source/WebKit/blackberry/Api/InRegionScroller.cpp:420
> > +bool InRegionScrollerPrivate::isValidScrollableLayerWebKitThread(WebCore::LayerWebKitThread* layerWebKitThread)
> 
> const
> 
> > Source/WebKit/blackberry/Api/InRegionScroller.cpp:432
> > +bool InRegionScrollerPrivate::isValidScrollableNode(WebCore::Node* node)
> 
> const
>

Fixing those. Good eye.
 
> > Source/WebKit/blackberry/Api/InRegionScroller.cpp:437
> > +    for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++)
> 
> IIRC calculateActiveAndShrinkCachedScrollableAreas shrink this vector. Does it need to be a for loop?

I think so. That shrinking is performed after we cast and use the layer, but we need to know if the layer still exists first.

[Mike Lattanzio]

Created attachment 179123 [details]
Patch

[Antonio Gomes]

Comment on attachment 179123 [details]
Patch

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

Please reupload with "Reviewed by antonio Gomes" and just request cq to land it.

r+, some nits though.

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:420
> +bool InRegionScrollerPrivate::isValidScrollableLayerWebKitThread(WebCore::LayerWebKitThread* layerWebKitThread) const

WebCore:: is needed?

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:428
> +    for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++)
> +        if (static_cast<InRegionScrollableArea*>(m_activeInRegionScrollableAreas[i])->cachedScrollableLayer() == layerWebKitThread)
> +            return true;
> +

for need {}

++i is preferred.

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:432
> +bool InRegionScrollerPrivate::isValidScrollableNode(WebCore::Node* node) const

WebCore is needed?

> Source/WebKit/blackberry/Api/InRegionScroller.cpp:439
> +    for (unsigned i = 0; i < m_activeInRegionScrollableAreas.size(); i++)
> +        if (static_cast<InRegionScrollableArea*>(m_activeInRegionScrollableAreas[i])->cachedScrollableNode() == node)
> +            return true;

ditto

++i is preferred.

[Mike Lattanzio]

Created attachment 179130 [details]
Patch

[Rob Buis]

Comment on attachment 179130 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 179130 [details]
Patch

Clearing flags on attachment: 179130

Committed r137532: <http://trac.webkit.org/changeset/137532>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.