Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for the race condition in bmalloc's BMScavenger caused by … #5315

Merged
merged 3 commits into from
May 24, 2024

Conversation

GlebNovodranPE
Copy link
Contributor

…WebKitBrowser plugin implementation unloading.

Sometimes BMScavenger crashes with a SIGSEGV in __pthread_cond_timedwait after WebKitBrowserPlugin implementation is unloaded, as it retains an invalidated condition variable.

The crashed thread callstack usually looks like:

5|0|libpthread-2.31.so|__condvar_dec_grefs|/usr/src/debug/glibc/2.31+gitAUTOINC+1094741224-r0/git/nptl/pthread_cond_wait.c|153|0x0
5|1|libpthread-2.31.so|__pthread_cond_timedwait|/usr/src/debug/glibc/2.31+gitAUTOINC+1094741224-r0/git/nptl/pthread_cond_wait.c|530|0x3
5|2|||||0xb21504e9
5|3|libstdc++.so.6.0.28|std::execute_native_thread_routine|/usr/src/debug/gcc-runtime/9.3.0-r0/arm-rdk-linux-gnueabi/libstdc++-v3/src/c++11/../../../../../../../../../../work-shared/gcc-9.3.0-r0/gcc-9.3.0/libstdc++-v3/src/c++11/thread.cc|80|0x3
5|4|libpthread-2.31.so|start_thread|/usr/src/debug/glibc/2.31+gitAUTOINC+1094741224-r0/git/nptl/pthread_create.c|477|0x1

This workaround is to prevent the plugin implementation unloading.

@CLAassistant
Copy link

CLAassistant commented May 20, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@GlebNovodranPE
Copy link
Contributor Author

@binuinbaraj , would you please review this?

emutavchi
emutavchi previously approved these changes May 20, 2024
@emutavchi emutavchi requested a review from modeveci May 20, 2024 13:21
@emutavchi
Copy link
Contributor

@modeveci could you please review as well?

binuinbaraj
binuinbaraj previously approved these changes May 20, 2024
@modeveci
Copy link
Contributor

@GlebNovodranPE , I will talk with browser team regarding this issue:
_Sometimes BMScavenger crashes with a SIGSEGV in _pthread_cond_timedwait after WebKitBrowserPlugin implementation is unloaded, as it retains an invalidated condition variable.

What do you mean by this preventing? When Thunder framework deactives plugin, what will happen?

@MFransen69 , could you please look at this PR?

@modeveci modeveci requested a review from MFransen69 May 22, 2024 18:12
@MFransen69
Copy link

Hi @GlebNovodranPE

Could you please explain a bit more on what BMScavenger is (googled but could not find much, guess it is a issue/leak detection tooL)? What processes is it attached to? (is in instrumented in, in the build or just an external tool?) e.g. to the in process part of the plugin (Thunder/WPEFramework), the OOP part (WPEProcess) or the Webkit processes itself?
And if you have this issue with Webkit don't you then have it also with other plugins or is BMScavenger only used for Webkit?
Like in your description it really sounds like a workaround. Unloading the libraries when a plugin is deactivated is an integral part of the Thunder architecture, e.g. for memory constraints, guaranteed correct (static) initialization, library replacement and not doing so will for example break our automated QA tools (we actually test this). So to me looks better to fix the underlying issue that deactivating the plugin works correctly also when BMScavenger is used.

@modeveci
Copy link
Contributor

@MFransen69 @GlebNovodranPE , I have talked with Marcel that bmalloc is coming from webkit. So questions regarding that area are clear.

This is the ticket of WPEWebkit to get support for proper fix from browser team: WebPlatformForEmbedded/WPEWebKit#1342

@GlebNovodranPE GlebNovodranPE dismissed stale reviews from binuinbaraj and emutavchi via 4f5c480 May 24, 2024 14:59
@emutavchi emutavchi merged commit 683b087 into rdkcentral:sprint/24Q2 May 24, 2024
6 checks passed
pradeep-raveendranpillai-infosys added a commit to LibertyGlobal/rdkservices that referenced this pull request Jun 24, 2024
mikolaj-staworzynski-red pushed a commit to LibertyGlobal/rdkservices that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants