Bug 101564 – [EFL][WK2] Make classes for client use WebContext instead of WKContext

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

[Jinwoo Song]

ewk_context has been refactored to use WebContext instead of WKContext in r133844. So client classes also should changed to use WebContext.

[Jinwoo Song]

Created attachment 172964 [details]
Patch

[Jinwoo Song]

Comment on attachment 172964 [details]
Patch

I'll upload other patch to align with bug 101565.

[Jinwoo Song]

Created attachment 172987 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 172987 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:69
> +    , m_batteryProvider(BatteryProvider::create(m_context.get()))

m_batteryProvider(BatteryProvider::create(m_context))

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:72
> +    , m_networkInfoProvider(NetworkInfoProvider::create(m_context.get()))

ditto

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:76
> +    , m_historyClient(ContextHistoryClientEfl::create(m_context.get()))

ditto

[Mikhail Pozdnyakov]

Comment on attachment 172987 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:83
> +    m_context.get()->batteryManagerProxy()->initializeProvider(&wkBatteryProvider);

m_context->batteryManagerProxy()

[Mikhail Pozdnyakov]

Comment on attachment 172987 [details]
Patch

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

smart pointer classes have -> operator overloaded!

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:60
> +    m_context.get()->batteryManagerProxy()->initializeProvider(0);

m_context->batteryManagerProxy()

[Chris Dumez]

Comment on attachment 172987 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Make classes for client use WebContext instead of WKContext

"Make context client classes..."

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:60
> +    m_context.get()->batteryManagerProxy()->initializeProvider(0);

useless ".get()"

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:82
> +    ASSERT(m_context.get()->batteryManagerProxy());

Ditto.

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:100
> +    ASSERT(m_context.get()->batteryManagerProxy());

Ditto.

> Source/WebKit2/UIProcess/efl/BatteryProvider.cpp:101
> +    m_context.get()->batteryManagerProxy()->providerDidChangeBatteryStatus(eventType, batteryStatus.get());

Ditto.

> Source/WebKit2/UIProcess/efl/NetworkInfoProvider.cpp:82
> +    ASSERT(m_context.get()->networkInfoManagerProxy());

Ditto.

> Source/WebKit2/UIProcess/efl/NetworkInfoProvider.cpp:88
> +    ASSERT(m_context.get()->networkInfoManagerProxy());

Ditto.....

[Mikhail Pozdnyakov]

BTW, you should reconsider your patch after https://bugs.webkit.org/show_bug.cgi?id=101565 lands

[Jinwoo Song]

Created attachment 173217 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 173217 [details]
Patch

looks ok

[WebKit Review Bot]

Comment on attachment 173217 [details]
Patch

Clearing flags on attachment: 173217

Committed r134043: <http://trac.webkit.org/changeset/134043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mikhail Pozdnyakov]

lead to crashes
ASSERTION FAILED: context
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/efl/BatteryProvider.cpp(73) : WebKit::BatteryProvider::BatteryProvider(WTF::PassRefPtr<WebKit::WebContext>)
1   0x2b42aab6132a WebKit::BatteryProvider::BatteryProvider(WTF::PassRefPtr<WebKit::WebContext>)
2   0x2b42aab61261 WebKit::BatteryProvider::create(WTF::PassRefPtr<WebKit::WebContext>)
3   0x2b42aab3edbe EwkContext::EwkContext(WTF::PassRefPtr<WebKit::WebContext>)
4   0x2b42aab3f278 EwkContext::create(WTF::PassRefPtr<WebKit::WebContext>)
5   0x2b42aab3f2de EwkContext::create()
6   0x2b42aab3f3a5 EwkContext::defaultContext()
7   0x2b42aab3f927 ewk_context_default_get
8   0x40fe67 EWK2UnitTest::EWK2UnitTestBase::SetUp()
9   0x2b42abcfef6f testing::Test::Run()
10  0x2b42abcff5d1 testing::internal::TestInfoImpl::Run()

after PassRefPtr has been assigned to smth you can never use it again, and this problem is pretty hard to see on review since the code which needs modification might not not in the patch.

It's a good habit to run tests or mini browser at least before uploading a patch.

[Mikhail Pozdnyakov]

link to crash log:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5702/steps/API%20tests/logs/stdio

[Mikhail Pozdnyakov]

and this is link to crash bug
https://bugs.webkit.org/show_bug.cgi?id=101742

[Jinwoo Song]

(In reply to comment #13)
> lead to crashes
> ASSERTION FAILED: context
> /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/efl/BatteryProvider.cpp(73) : WebKit::BatteryProvider::BatteryProvider(WTF::PassRefPtr<WebKit::WebContext>)
> 1   0x2b42aab6132a WebKit::BatteryProvider::BatteryProvider(WTF::PassRefPtr<WebKit::WebContext>)
> 2   0x2b42aab61261 WebKit::BatteryProvider::create(WTF::PassRefPtr<WebKit::WebContext>)
> 3   0x2b42aab3edbe EwkContext::EwkContext(WTF::PassRefPtr<WebKit::WebContext>)
> 4   0x2b42aab3f278 EwkContext::create(WTF::PassRefPtr<WebKit::WebContext>)
> 5   0x2b42aab3f2de EwkContext::create()
> 6   0x2b42aab3f3a5 EwkContext::defaultContext()
> 7   0x2b42aab3f927 ewk_context_default_get
> 8   0x40fe67 EWK2UnitTest::EWK2UnitTestBase::SetUp()
> 9   0x2b42abcfef6f testing::Test::Run()
> 10  0x2b42abcff5d1 testing::internal::TestInfoImpl::Run()
> 
> after PassRefPtr has been assigned to smth you can never use it again, and this problem is pretty hard to see on review since the code which needs modification might not not in the patch.
> 
> It's a good habit to run tests or mini browser at least before uploading a patch.

My bad. I usually run API test before submitting patch, but I did not run in this time as the patch was rebased many times. I should have tested the API test and Minibrowser and I'll double check next time. I'm sorry for bothering you.