Bug 95636 – [EFL] Fix e_dbus_shutdown() error when exiting the Minibrowser

• Component: WebKit2
• Status: RESOLVED FIXED

[Jinwoo Song]

The destructor of BatteryProviderEfl should not call stopUpdating() 
because it invokes e_ukit_shutdown() even though e_ukit_init() is not called.
The e_ukit_shutdown() calls the e_dbus_shutdown() and it returns error  
that the init is not called properly. 

EAPI int
e_dbus_shutdown(void)
{
   if (_edbus_init_count <= 0)
     {
        EINA_LOG_ERR("Init count not greater than 0 in shutdown.");
        return 0;
     }
  ...
}

[Jinwoo Song]

Created attachment 161818 [details]
patch.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 161818 [details]
patch.

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

> Source/WebKit2/ChangeLog:9
> +        The destructor of BatteryProviderEfl should not call stopUpdating() 
> +        because it invokes e_ukit_shutdown() even though e_ukit_init() is not called.

e_ukit_init() is called in BatteryProviderEFl::startUpdating(), so there seems to be some other problem here.

[Chris Dumez]

Comment on attachment 161818 [details]
patch.

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

>> Source/WebKit2/ChangeLog:9
>> +        because it invokes e_ukit_shutdown() even though e_ukit_init() is not called.
> 
> e_ukit_init() is called in BatteryProviderEFl::startUpdating(), so there seems to be some other problem here.

Well, the problem is possibly that stopUpdating() is called several times?

> Source/WebKit2/UIProcess/API/efl/BatteryProvider.cpp:-55
> -    m_provider.stopUpdating();

Could we maybe add a boolean property to the class to know if we are updating or not? This way we should call m_provider.stopUpdating() only we we are still updating (i.e. stopUpdating() was not explicitly called).
I think this would be more reliable.

[Chris Dumez]

Actually, the fix seems consistent with the WebKit1 implementation so OK.

In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice.

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 161818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161818&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        The destructor of BatteryProviderEfl should not call stopUpdating() 
> > +        because it invokes e_ukit_shutdown() even though e_ukit_init() is not called.
> 
> e_ukit_init() is called in BatteryProviderEFl::startUpdating(), so there seems to be some other problem here.

Currently, startUpdating() is not called when the Minibrowser launches.

[Jinwoo Song]

(In reply to comment #4)
> Actually, the fix seems consistent with the WebKit1 implementation so OK.
> 
> In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice.

I totally agree you that we need such code and would like you to add that.
Thanks for your review.

[Kihong Kwon]

(In reply to comment #4)
> Actually, the fix seems consistent with the WebKit1 implementation so OK.
> 
> In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice.

I agree with this.
IMO, We need to add a flag to check for starting update.

[Jinwoo Song]

Created attachment 161916 [details]
patch

[WebKit Review Bot]

Comment on attachment 161916 [details]
patch

Attachment 161916 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13755029

[Jinwoo Song]

(In reply to comment #7)
> (In reply to comment #4)
> > Actually, the fix seems consistent with the WebKit1 implementation so OK.
> > 
> > In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice.
> 
> I agree with this.
> IMO, We need to add a flag to check for starting update.

I did not add a new flag to check the updating status but used the existing one, 
m_timer in the BatteryProviderEfl.h. By checking if the m_timer is active or not,
stopUpdating() is executed only when we are updating. 

Also, I added stopUpdating() in the destructor of BatteryClientEfl.cpp as we can 
check if we are updating or not.

[Peter Beverloo (cr-android ews)]

Comment on attachment 161916 [details]
patch

Attachment 161916 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13734463

[Chris Dumez]

Comment on attachment 161916 [details]
patch

LGTM. Thanks, this version is much better IMHO.

[Chris Dumez]

Why didn't you set review flag?

[Jinwoo Song]

(In reply to comment #13)
> Why didn't you set review flag?

I changed the bug title to match the ChangeLog and set review flag.

[Jinwoo Song]

Created attachment 161951 [details]
patch

[Gyuyoung Kim]

Comment on attachment 161951 [details]
patch

Looks fine.

[WebKit Review Bot]

Comment on attachment 161951 [details]
patch

Clearing flags on attachment: 161951

Committed r127436: <http://trac.webkit.org/changeset/127436>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.