Bug 119333 - [EFL] REGRESSION after r153432: some accessibility tests crash.
Summary: [EFL] REGRESSION after r153432: some accessibility tests crash.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-31 03:48 PDT by Krzysztof Czech
Modified: 2013-08-08 00:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch proposal (2.44 KB, patch)
2013-07-31 04:09 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (1.80 KB, patch)
2013-08-07 13:42 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2013-07-31 03:48:47 PDT
Crash log:

0xb70267b7
0xb717b288
0xb3fcf208 g_object_ref_sink
0xaf19b444
0xaf19d023 WTR::AccessibilityUIElement::allAttributes()
0xaf18ec57 WTR::JSAccessibilityUIElement::allAttributes(OpaqueJSContext const*, OpaqueJSVal    ue*, OpaqueJSValue*, unsigned int, OpaqueJSValue const* const*, OpaqueJSValue const**)
0xb6c6b32b long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState    *)
0xb70182d7
0xb701f648
0xb7025f33
0xb6e402bb JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*)
Comment 1 Krzysztof Czech 2013-07-31 03:50:47 PDT
It seems AccessibilityUIElement does not have parent.
Comment 2 Mario Sanchez Prada 2013-07-31 03:56:08 PDT
Oops! I forgot to add that null check, sorry about that

I'll be posting a patch right away
Comment 3 Mario Sanchez Prada 2013-07-31 04:09:37 PDT
Created attachment 207832 [details]
Patch proposal

Let's hope I have not made more mistakes
Comment 4 Mario Sanchez Prada 2013-07-31 04:10:18 PDT
Adding Gustavo as reviewer for this -almost- one liner patch
Comment 5 Mario Sanchez Prada 2013-08-02 08:27:22 PDT
Comment on attachment 207832 [details]
Patch proposal

Thanks for the review. Adding it to the commit queue...
Comment 6 WebKit Commit Bot 2013-08-02 08:51:28 PDT
Comment on attachment 207832 [details]
Patch proposal

Clearing flags on attachment: 207832

Committed r153651: <http://trac.webkit.org/changeset/153651>
Comment 7 WebKit Commit Bot 2013-08-02 08:51:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Krzysztof Czech 2013-08-05 02:30:42 PDT
I'm reopening this bug. Crash still occurs, I guess even on gtk port as well.
Comment 9 Krzysztof Czech 2013-08-05 03:31:53 PDT
The crash seems to appear while calling parent->platformUIElement(). This method returns PlatformUIElement type which is a GRefPtr<AtkObject>.
I believe in this context:
AtkObject* atkParent = parent ? parent->platformUIElement().get() : 0;

platformUIElement().get(), before get() is called, the temporary PlatformUIElement is created and refGPtr(ptr) is called where ptr is a GRefPtr<AtkObject>. I think, that's way g_object_ref_sink protests.

I guess platformUIElement could be specialized for ATK so that it could return AtkObject*.
Comment 10 Krzysztof Czech 2013-08-05 03:41:35 PDT
While testing this approach with specialized platformUIElement (I called it platformUIElementAtk()), I did find this crash, but another one appeared:

1 0xb7033767
2 0xb7186288
3   0xb3ed5627 atk_object_get_role
4   0xafb9b568
5   0xafb9d173 WTR::AccessibilityUIElement::allAttributes()
6   0xafb8ec47 WTR::JSAccessibilityUIElement::allAttributes(OpaqueJSContext const*, OpaqueJSVal    ue*, OpaqueJSValue*, unsigned int, OpaqueJSValue const* const*, OpaqueJSValue const**)
7   0xb6c79292 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState    *)
8   0xb7025035
9   0xb702c448
10  0xb7032ee3
11  0xb6e4d3cb JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*)
Comment 11 Krzysztof Czech 2013-08-05 03:42:40 PDT
(In reply to comment #10)
> ... I did find this crash ...
I did not find this crash
Comment 12 Mario Sanchez Prada 2013-08-05 08:19:18 PDT
Thanks Krzysztof for reporting that the issue has not been properly fixed yet. I can take a look to it tomorrow if you want, but please confirm that will be fine since I see you have been already doing some investigation and I don't want to collide with your efforts, should you were planning to work on this.
Comment 13 Krzysztof Czech 2013-08-05 08:27:10 PDT
(In reply to comment #12)
> Thanks Krzysztof for reporting that the issue has not been properly fixed yet. I can take a look to it tomorrow if you want, but please confirm that will be fine since I see you have been already doing some investigation and I don't want to collide with your efforts, should you were planning to work on this.
Yes, I will be fine if you take a look at this issue. I wrote some suppositions, but I'm not sure whether they hit the point, they may be wrong. I just looked at this briefly.
Comment 14 Mario Sanchez Prada 2013-08-05 08:42:39 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Thanks Krzysztof for reporting that the issue has not been properly fixed yet. I can take a look to it tomorrow if you want, but please confirm that will be fine since I see you have been already doing some investigation and I don't want to collide with your efforts, should you were planning to work on this.
> Yes, I will be fine if you take a look at this issue. I wrote some suppositions, but I'm not sure whether they hit the point, they may be wrong. I just looked at this briefly.

Ok, fair enough. I'll work tomorrow on that then. Today I'm just almost dead because of this GUADEC conference :)
Comment 15 Mario Sanchez Prada 2013-08-07 13:42:33 PDT
Created attachment 208297 [details]
Patch proposal

The problem seems to be more simple in the end: We just need to store a RefPtr for the parent instead of the raw pointer.
Comment 16 WebKit Commit Bot 2013-08-07 14:12:31 PDT
Comment on attachment 208297 [details]
Patch proposal

Clearing flags on attachment: 208297

Committed r153798: <http://trac.webkit.org/changeset/153798>
Comment 17 WebKit Commit Bot 2013-08-07 14:12:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Krzysztof Czech 2013-08-08 00:17:06 PDT
Thanks Mario