Add a test that confirms there's no crash on accessing properties of AccesibilityObject at the init…

Add a test that confirms there's no crash on accessing properties of AccesibilityObject at the initial state

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

[hajimehoshi, 2014-03-28 06:11:09]

PTAL

[dmazzoni, 2014-03-28 06:30:32]

https://codereview.chromium.org/216343002/diff/20001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/resources/window-property-collector.js (left):

https://codereview.chromium.org/216343002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/resources/window-property-collector.js:81: // FIXME:
We can't access accessibilityController's properties here
It looks like you're deleting the FIXME, but the step that skips
accessibilityController is still there

https://codereview.chromium.org/216343002/diff/20001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/216343002/diff/20001/Source/web/WebAXObject.c...
Source/web/WebAXObject.cpp:706: if (m_private->isTextControl())
I'm assuming this fixes the crash, but it doesn't make sense and isn't helpful
to fix it here. Can you find the underlying cause of the crash? I'm assuming
it's in AXRenderObject.cpp or further downstream.

[hajimehoshi, 2014-03-28 06:45:08]

https://codereview.chromium.org/216343002/diff/20001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/216343002/diff/20001/Source/web/WebAXObject.c...
Source/web/WebAXObject.cpp:706: if (m_private->isTextControl())
On 2014/03/28 06:30:32, dmazzoni wrote:
> I'm assuming this fixes the crash, but it doesn't make sense and isn't helpful
> to fix it here. Can you find the underlying cause of the crash? I'm assuming
> it's in AXRenderObject.cpp or further downstream.

I agree with you.

The cause is that there is an assertion at AXRendererObject::selectedTextRange
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
while AXRendererObject::selectedTextRange could be accessed from any kinds of
AXRendererObjects. I'm afraid I wan't sure these asserts could be eliminated.

BTW, a similar code exists at
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/a...
in terms of 'if' check before accessing proprties.

[dmazzoni, 2014-03-28 06:54:52]

On Thu, Mar 27, 2014 at 11:45 PM, <hajimehoshi@chromium.org> wrote:

> The cause is that there is an assertion at
>
AXRendererObject::selectedTextRange
> (https://code.google.com/p/chromium/codesearch#chromium/
> src/third_party/WebKit/Source/core/accessibility/
> AXRenderObject.cpp&sq=package:chromium&type=cs&l=1571&rcl=1395936118),
> while AXRendererObject::selectedTextRange could be accessed from any
> kinds of AXRendererObjects. I'm afraid I wan't sure these asserts could
> be eliminated.
>

Ah, got it.

Yeah, let's eliminate the assertions and just replace them with runtime
checks that return a default value.


>
> BTW, a similar code exists at
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/renderer/accessibility/accessibility_
> node_serializer.cc&sq=package:chromium&type=cs&l=225&rcl=1395936118
> in terms of 'if' check before accessing proprties.
>

Yes. That one is fine as an optimization - but I don't think we should have
an assert if you call a function on an object that doesn't normally support
it. Most of the accessibility code works this way - it just returns a
default value. These asserts are the exception and we should fix them.

Thanks!
- Dominic

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[hajimehoshi, 2014-03-28 07:23:46]

Thanks! PTAL

https://codereview.chromium.org/216343002/diff/20001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/resources/window-property-collector.js (left):

https://codereview.chromium.org/216343002/diff/20001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/resources/window-property-collector.js:81: // FIXME:
We can't access accessibilityController's properties here
On 2014/03/28 06:30:32, dmazzoni wrote:
> It looks like you're deleting the FIXME, but the step that skips
> accessibilityController is still there

Done.

[dmazzoni, 2014-03-28 16:23:49]

lgtm

Great, thanks!

[hajimehoshi, 2014-03-28 16:40:52]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:41:02]

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

[commit-bot: I haz the power, 2014-03-28 17:08:04]

Change committed as 170340