Bug 97528 – [EFL][WK2] Clear provider on destructor of {Vibration,Battery,NetworkInfo}Provider.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

Sometimes there is a crash on ewk_context_new and the callstack is as below.

#0  0xb7146fb8 in VibrationProvider::cancelVibration (this=0xadb058b0) at Source/WebKit2/UIProcess/API/efl/VibrationProvider.cpp:99
#1  0xb7146d14 in cancelVibrationCallback (clientInfo=0xadb058b0) at Source/WebKit2/UIProcess/API/efl/VibrationProvider.cpp:65
#2  0xb70784e8 in WebKit::WebVibrationProvider::cancelVibration (this=0xadb128f4, vibration=0xadb128e8) at Source/WebKit2/UIProcess/WebVibrationProvider.cpp:49
#3  0xb707873f in WebKit::WebVibrationProxy::cancelVibration (this=0xadb128e8) at Source/WebKit2/UIProcess/WebVibrationProxy.cpp:71
#4  0xb7078663 in WebKit::WebVibrationProxy::invalidate (this=0xadb128e8) at Source/WebKit2/UIProcess/WebVibrationProxy.cpp:51
#5  0xb6ff550c in WebKit::WebContext::~WebContext (this=0xadb122b0, __in_chrg=<optimized out>) at Source/WebKit2/UIProcess/WebContext.cpp:226
#6  0xb6ff5799 in WebKit::WebContext::~WebContext (this=0xadb122b0, __in_chrg=<optimized out>) at Source/WebKit2/UIProcess/WebContext.cpp:237
#7  0xb6f994eb in WTF::ThreadSafeRefCounted<WebKit::APIObject>::deref (this=0xadb122b4) at Source/WTF/wtf/ThreadSafeRefCounted.h:137
#8  0xb6fdac48 in WKRelease (typeRef=0xadb122b0) at Source/WebKit2/Shared/API/c/WKType.cpp:47
#9  0xb714bc81 in WebKit::WKRetainPtr<OpaqueWKContext const*>::~WKRetainPtr (this=0xadb1292c, __in_chrg=<optimized out>)
    at Source/WebKit2/UIProcess/API/cpp/WKRetainPtr.h:86
#10 0xb714bb9f in _Ewk_Context::~_Ewk_Context (this=0xadb12928, __in_chrg=<optimized out>) at Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:109
#11 0xb714ac66 in ewk_context_unref (ewkContext=0xadb12928) at Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:136
#12 0x08054627 in EWK2UnitTestBase_ewk_context_new_Test::TestBody (this=0xadb00860) at Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:159
#13 0xb69dc828 in testing::Test::Run (this=0xadb00860) at Source/ThirdParty/gtest/src/gtest.cc:2095
#14 0xb69dce83 in testing::internal::TestInfoImpl::Run (this=0x8063010) at Source/ThirdParty/gtest/src/gtest.cc:2314
#15 0xb69dd3e5 in testing::TestCase::Run (this=0x8062c80) at Source/ThirdParty/gtest/src/gtest.cc:2420
#16 0xb69e1b38 in testing::internal::UnitTestImpl::RunAllTests (this=0x8062a48) at Source/ThirdParty/gtest/src/gtest.cc:4024
#17 0xb69e0a34 in testing::UnitTest::Run (this=0xb6a19b20) at Source/ThirdParty/gtest/src/gtest.cc:3687
#18 0x08059582 in main (argc=1, argv=0xbffff1b4) at Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestMain.cpp:52

Before this callstack ~VibrationProvider() is called but provider for vibration is not cleared in the function.
So the dangling pointer for the VibrationProvider is accessed at the #2 frame.

[Byungwoo Lee]

Created attachment 165531 [details]
Patch

[Byungwoo Lee]

Created attachment 165947 [details]
Patch

[Chris Dumez]

Comment on attachment 165947 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:119
> +        batteryProvider.clear();

I don't believe this is needed.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:123
> +        vibrationProvider.clear();

Ditto.

[Byungwoo Lee]

(In reply to comment #3)
> (From update of attachment 165947 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165947&action=review
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:119
> > +        batteryProvider.clear();
> I don't believe this is needed.
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:123
> > +        vibrationProvider.clear();
> Ditto.

'context' has WebVibrationProxy and the pointer is saved in the VibrationProvider.
How about the 'context' is deleted before the 'vibration'?
Then the destructor will access dangling pointer.

I also think this explicit clear() looks nice.

So I'm checking some other way to arrange deletion order.
(e.g. instead of having context as member variable, Ewk_Context inherits it.)

How about your opinion?

[Byungwoo Lee]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 165947 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=165947&action=review
> > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:119
> > > +        batteryProvider.clear();
> > I don't believe this is needed.
> > > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:123
> > > +        vibrationProvider.clear();
> > Ditto.
> 'context' has WebVibrationProxy and the pointer is saved in the VibrationProvider.
> How about the 'context' is deleted before the 'vibration'?
> Then the destructor will access dangling pointer.
*destructor of VibrationProvider*
> I also think this explicit clear() looks nice.
> So I'm checking some other way to arrange deletion order.
> (e.g. instead of having context as member variable, Ewk_Context inherits it.)
> How about your opinion?

[Chris Dumez]

Comment on attachment 165947 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:119
>>>> +        batteryProvider.clear();
>>> 
>>> I don't believe this is needed.
>> 
>> 'context' has WebVibrationProxy and the pointer is saved in the VibrationProvider.
>> How about the 'context' is deleted before the 'vibration'?
>> Then the destructor will access dangling pointer.
>> 
>> I also think this explicit clear() looks nice.
>> 
>> So I'm checking some other way to arrange deletion order.
>> (e.g. instead of having context as member variable, Ewk_Context inherits it.)
>> 
>> How about your opinion?
> 
> *destructor of VibrationProvider*

I don't think explicitly calling clear() here is nice. It should not be required either. If it is required, then we need to fix this in some more reliable way.

I still don't understand what the problem is:
1. 'context' has WebVibrationProxy member, yes, it keeps a reference to it.
2. The pointer is saved in VibrationProvider. You mean WKVibrationRef ?

I don't see the problem with WKVibrationRef since it is stored in a WKRetainPtr<> inside VibrationProvider. It is ref counted so the WKVibrationRef should still be valid in VibrationProvider, no matter what.

[Byungwoo Lee]

(In reply to comment #6)
> (From update of attachment 165947 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165947&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:119
> >>>> +        batteryProvider.clear();
> >>> 
> >>> I don't believe this is needed.
> >> 
> >> 'context' has WebVibrationProxy and the pointer is saved in the VibrationProvider.
> >> How about the 'context' is deleted before the 'vibration'?
> >> Then the destructor will access dangling pointer.
> >> 
> >> I also think this explicit clear() looks nice.
> >> 
> >> So I'm checking some other way to arrange deletion order.
> >> (e.g. instead of having context as member variable, Ewk_Context inherits it.)
> >> 
> >> How about your opinion?
> > 
> > *destructor of VibrationProvider*
> 
> I don't think explicitly calling clear() here is nice. It should not be required either. If it is required, then we need to fix this in some more reliable way.
Yes above is typo. I missed 'not' :)

> 
> I still don't understand what the problem is:
> 1. 'context' has WebVibrationProxy member, yes, it keeps a reference to it.
> 2. The pointer is saved in VibrationProvider. You mean WKVibrationRef ?
> 
> I don't see the problem with WKVibrationRef since it is stored in a WKRetainPtr<> inside VibrationProvider. It is ref counted so the WKVibrationRef should still be valid in VibrationProvider, no matter what.

Yes as you said, WebVibrationProvider (== WebVibrationProxy) will not be deleted at that time.
But originally, WebVibrationProxy is a member of the WebContext and at that time, WebVibrationProxy will be alive without WebContext.
(And WebVibrationProxy has a raw pointer of WebContext. This can be a dangling)

I searched about delete order of class member.
I'm not sure but 'delete the last member first' might be the rule.
(Is there anyone who knows about this?)

If so, the above code can be deleted as you recommended.

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 165947 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=165947&action=review
> > 
> > >>>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:119
> > >>>> +        batteryProvider.clear();
> > >>> 
> > >>> I don't believe this is needed.
> > >> 
> > >> 'context' has WebVibrationProxy and the pointer is saved in the VibrationProvider.
> > >> How about the 'context' is deleted before the 'vibration'?
> > >> Then the destructor will access dangling pointer.
> > >> 
> > >> I also think this explicit clear() looks nice.
> > >> 
> > >> So I'm checking some other way to arrange deletion order.
> > >> (e.g. instead of having context as member variable, Ewk_Context inherits it.)
> > >> 
> > >> How about your opinion?
> > > 
> > > *destructor of VibrationProvider*
> > 
> > I don't think explicitly calling clear() here is nice. It should not be required either. If it is required, then we need to fix this in some more reliable way.
> Yes above is typo. I missed 'not' :)
> 
> > 
> > I still don't understand what the problem is:
> > 1. 'context' has WebVibrationProxy member, yes, it keeps a reference to it.
> > 2. The pointer is saved in VibrationProvider. You mean WKVibrationRef ?
> > 
> > I don't see the problem with WKVibrationRef since it is stored in a WKRetainPtr<> inside VibrationProvider. It is ref counted so the WKVibrationRef should still be valid in VibrationProvider, no matter what.
> 
> Yes as you said, WebVibrationProvider (== WebVibrationProxy) will not be deleted at that time.
> But originally, WebVibrationProxy is a member of the WebContext and at that time, WebVibrationProxy will be alive without WebContext.
> (And WebVibrationProxy has a raw pointer of WebContext. This can be a dangling)
> 
> I searched about delete order of class member.
> I'm not sure but 'delete the last member first' might be the rule.
> (Is there anyone who knows about this?)
> 
> If so, the above code can be deleted as you recommended.

WebVibrationProxy is a member of WebContext so it is not a problem if WebVibrationProxy contains a raw pointer to WebContext. All the proxies keep a raw pointer to WebContext.

[Byungwoo Lee]

> WebVibrationProxy is a member of WebContext so it is not a problem if WebVibrationProxy contains a raw pointer to WebContext. All the proxies keep a raw pointer to WebContext.

Yes, I'm not focusing about the raw pointer itself.

If WebContext can be deleted before the VibrationProvider, WebVibrationProxy will be still alive without the WebContext. then the instance can has a dangling pointer for the WebContext because it has a raw pointer.

I think that the issue is the 'deleting order' of WebContext and VibrationProvider. 
(And it seems that WebContext will be deleted after VibrationProvider)

Do you have a clear point about the deleting order?

[Chris Dumez]

(In reply to comment #9)
> > WebVibrationProxy is a member of WebContext so it is not a problem if WebVibrationProxy contains a raw pointer to WebContext. All the proxies keep a raw pointer to WebContext.
> 
> Yes, I'm not focusing about the raw pointer itself.
> 
> If WebContext can be deleted before the VibrationProvider, WebVibrationProxy will be still alive without the WebContext. then the instance can has a dangling pointer for the WebContext because it has a raw pointer.

VibrationProvider has no link with WebContext, so it does not matter the order in which they are deleted.

WebContext owns WebVibrationProxy. WebVibration owns WebVibrationProvider.
WebVibrationProvider merely calls callback functions from WKBatteryProvider.

It looks like you are confusing WebVibrationProvider and VibrationProvider.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > > WebVibrationProxy is a member of WebContext so it is not a problem if WebVibrationProxy contains a raw pointer to WebContext. All the proxies keep a raw pointer to WebContext.
> > 
> > Yes, I'm not focusing about the raw pointer itself.
> > 
> > If WebContext can be deleted before the VibrationProvider, WebVibrationProxy will be still alive without the WebContext. then the instance can has a dangling pointer for the WebContext because it has a raw pointer.

[Sorry I repeat because I made a typo in one name and it is important to be clear here]

VibrationProvider has no link with WebContext, so it does not matter the order in which they are deleted.

WebContext owns WebVibrationProxy. WebVibrationProxy owns WebVibrationProvider.
WebVibrationProvider merely calls callback functions from WKBatteryProvider.

It looks like you are confusing WebVibrationProvider and VibrationProvider.

[Chris Dumez]

I believe I have (finally) understood the problem.

The issue is with the following member in BatteryManager:
 WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;

This means BatteryManager keeps a reference to the WebBatteryManagerProxy and WebContext keeps another reference to WebBatteryManagerProxy.

This is indeed wrong. WebContext should own the WebBatteryManagerProxy and it should not be possible that WebBatteryManagerProxy exists after WebContext has been destroyed.

A solution for this would be to pass a WKContextRef to the our EFL providers (instead of WKBatteryManagerRef for example).

1. We can have keep a member in BatteryManager such as:
 WKRetainPtr<WKContextRef> m_context;
instead of
 WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;

and we can call WKContextGetBatteryManager(contextRef.get()) to get the battery manager when needed.

What do you think about this? I think it is simpler than calling WKRetainPtr::clear() explicitly to enforce a given destruction order. It is also less bug prone.

[Byungwoo Lee]

(In reply to comment #12)
> I believe I have (finally) understood the problem.
> 
> The issue is with the following member in BatteryManager:
>  WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;
> 
> This means BatteryManager keeps a reference to the WebBatteryManagerProxy and WebContext keeps another reference to WebBatteryManagerProxy.
> 
> This is indeed wrong. WebContext should own the WebBatteryManagerProxy and it should not be possible that WebBatteryManagerProxy exists after WebContext has been destroyed.
Yes~ Thats what I told about VibrationProvider. Both have same problem.

> 
> A solution for this would be to pass a WKContextRef to the our EFL providers (instead of WKBatteryManagerRef for example).
> 
> 1. We can have keep a member in BatteryManager such as:
>  WKRetainPtr<WKContextRef> m_context;
> instead of
>  WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;
> 
> and we can call WKContextGetBatteryManager(contextRef.get()) to get the battery manager when needed.
> 
> What do you think about this? I think it is simpler than calling WKRetainPtr::clear() explicitly to enforce a given destruction order. It is also less bug prone.

Yes I have had a doubt about this unclear relation between VibrationProvider and WebContext. (BatteryManager also)

Your suggestion looks nicer than the explicit clear() and removes the unclear point.
I'll change it.

[Byungwoo Lee]

(In reply to comment #13)
> (In reply to comment #12)
> > I believe I have (finally) understood the problem.
> > 
> > The issue is with the following member in BatteryManager:
> >  WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;
> > 
> > This means BatteryManager keeps a reference to the WebBatteryManagerProxy and WebContext keeps another reference to WebBatteryManagerProxy.
> > 
> > This is indeed wrong. WebContext should own the WebBatteryManagerProxy and it should not be possible that WebBatteryManagerProxy exists after WebContext has been destroyed.
> Yes~ Thats what I told about VibrationProvider. Both have same problem.
> 
> > 
> > A solution for this would be to pass a WKContextRef to the our EFL providers (instead of WKBatteryManagerRef for example).
> > 
> > 1. We can have keep a member in BatteryManager such as:
> >  WKRetainPtr<WKContextRef> m_context;
> > instead of
> >  WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;
> > 
> > and we can call WKContextGetBatteryManager(contextRef.get()) to get the battery manager when needed.
> > 
> > What do you think about this? I think it is simpler than calling WKRetainPtr::clear() explicitly to enforce a given destruction order. It is also less bug prone.
> 
> Yes I have had a doubt about this unclear relation between VibrationProvider and WebContext. (BatteryManager also)
> 
> Your suggestion looks nicer than the explicit clear() and removes the unclear point.
> I'll change it.
I'm working on the above on VibrationProvider, BatteryProvider, and NetworkInfoProvider and will upload new patch for it.

[Byungwoo Lee]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > I believe I have (finally) understood the problem.
> > > 
> > > The issue is with the following member in BatteryManager:
> > >  WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;
> > > 
> > > This means BatteryManager keeps a reference to the WebBatteryManagerProxy and WebContext keeps another reference to WebBatteryManagerProxy.
> > > 
> > > This is indeed wrong. WebContext should own the WebBatteryManagerProxy and it should not be possible that WebBatteryManagerProxy exists after WebContext has been destroyed.
> > Yes~ Thats what I told about VibrationProvider. Both have same problem.
> > 
> > > 
> > > A solution for this would be to pass a WKContextRef to the our EFL providers (instead of WKBatteryManagerRef for example).
> > > 
> > > 1. We can have keep a member in BatteryManager such as:
> > >  WKRetainPtr<WKContextRef> m_context;
> > > instead of
> > >  WKRetainPtr<WKBatteryManagerRef> m_wkBatteryManager;
> > > 
> > > and we can call WKContextGetBatteryManager(contextRef.get()) to get the battery manager when needed.
> > > 
> > > What do you think about this? I think it is simpler than calling WKRetainPtr::clear() explicitly to enforce a given destruction order. It is also less bug prone.
> > 
> > Yes I have had a doubt about this unclear relation between VibrationProvider and WebContext. (BatteryManager also)
> > 
> > Your suggestion looks nicer than the explicit clear() and removes the unclear point.
> > I'll change it.
> I'm working on the above on VibrationProvider, BatteryProvider, and NetworkInfoProvider and will upload new patch for it.

I think this need to be handled with separated bug, so I made a new bug for the relation problem : bug 97839

I'll discard the second patch and use the initial patch (that only has clear code)

[Chris Dumez]

Comment on attachment 165531 [details]
Patch

LGTM.

[Kenneth Rohde Christiansen]

Why is the last patch not up for review? Should it be?

[Chris Dumez]

(In reply to comment #17)
> Why is the last patch not up for review? Should it be?

No, the second patch's approach was discussed and we agreed it should be done another way (See Bug 97839).

[WebKit Review Bot]

Comment on attachment 165531 [details]
Patch

Clearing flags on attachment: 165531

Committed r129866: <http://trac.webkit.org/changeset/129866>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.