Closed Bug 1044963 Opened 10 years ago Closed 10 years ago

IsHeadphoneEventFromInputDev should not call InitializeResourceIfNeed directly from main thread

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S1 (1aug)

People

(Reporter: slee, Assigned: kershaw)

References

Details

Attachments

(1 file, 1 obsolete file)

[Final] Avoid calling RegisterUeventListener in main thread
1.64 KB, patch
kershaw
: review+
Details | Diff | Splinter Review 
IsHeadphoneEventFromInputDev calls RegisterUeventListener from main thread in [1] which is prohibited. It also has potential memory leakage problem, ex IsHeadphoneEventFromInputDev or NotifySwitchStateFromInputDevice is the latest one to call InitializeResourceIfNeed.

Here is the call stack
#0  mozilla::hal_impl::RegisterUeventListener (aObserver=0x4464ae80) at /home/steven/workspace/b2g/gecko-dev/hal/gonk/UeventPoller.cpp:209
#1  0x40f71efe in InitializeResourceIfNeed () at /home/steven/workspace/b2g/gecko-dev/hal/gonk/GonkSwitch.cpp:393
#2  0x40f71f3a in mozilla::hal_impl::IsHeadphoneEventFromInputDev () at /home/steven/workspace/b2g/gecko-dev/hal/gonk/GonkSwitch.cpp:474
#3  0x40f684ec in mozilla::hal::IsHeadphoneEventFromInputDev () at /home/steven/workspace/b2g/gecko-dev/hal/Hal.cpp:1245
#4  0x4185b8f4 in nsAppShell::InitInputDevices (this=0x0) at /home/steven/workspace/b2g/gecko-dev/widget/gonk/nsAppShell.cpp:1059
#5  0x4185bd64 in nsAppShell::NotifyScreenInitialized () at /home/steven/workspace/b2g/gecko-dev/widget/gonk/nsAppShell.cpp:1150
#6  0x4185e042 in nsWindow (this=0x402674e0) at /home/steven/workspace/b2g/gecko-dev/widget/gonk/nsWindow.cpp:155
#7  0x4185d328 in nsWindowConstructor (aOuter=<value optimized out>, aIID=..., aResult=0xbe83c978) at /home/steven/workspace/b2g/gecko-dev/widget/gonk/nsWidgetFactory.cpp:51
#8  0x40c4f500 in mozilla::GenericFactory::CreateInstance (this=<value optimized out>, aOuter=<value optimized out>, aIID=<value optimized out>, aResult=0x44649fe0) at /home/steven/workspace/b2g/gecko-dev/xpcom/glue/GenericFactory.cpp:17
#9  0x40c391f4 in nsComponentManagerImpl::CreateInstance (this=<value optimized out>, aClass=..., aDelegate=0x0, aIID=..., aResult=0xbe83c978) at /home/steven/workspace/b2g/gecko-dev/xpcom/components/nsComponentManager.cpp:998
#10 0x40c51094 in CallCreateInstance (aCID=..., aDelegate=0x0, aIID=..., aResult=0xbe83c978) at /home/steven/workspace/b2g/gecko-dev/xpcom/glue/nsComponentManagerUtils.cpp:136
#11 0x40c510bc in nsCreateInstanceByCID::operator() (this=0xbe83c940, aIID=<value optimized out>, aInstancePtr=0xbe83c978) at /home/steven/workspace/b2g/gecko-dev/xpcom/glue/nsComponentManagerUtils.cpp:183
#12 0x41e64830 in nsCOMPtr<nsIWidget>::assign_from_helper (this=0x4023ba00, aParent=0x0, aOpener=<value optimized out>, aUrl=0x0, aInitialWidth=1, aInitialHeight=1, aIsHiddenWindow=<value optimized out>, aOpeningTab=0x0, widgetInitData=...)
    at ../../../dist/include/nsCOMPtr.h:1250
#13 nsCOMPtr<nsIWidget>::operator= (this=0x4023ba00, aParent=0x0, aOpener=<value optimized out>, aUrl=0x0, aInitialWidth=1, aInitialHeight=1, aIsHiddenWindow=<value optimized out>, aOpeningTab=0x0, widgetInitData=...) at ../../../dist/include/nsCOMPtr.h:749
#14 nsWebShellWindow::Initialize (this=0x4023ba00, aParent=0x0, aOpener=<value optimized out>, aUrl=0x0, aInitialWidth=1, aInitialHeight=1, aIsHiddenWindow=<value optimized out>, aOpeningTab=0x0, widgetInitData=...)
    at /home/steven/workspace/b2g/gecko-dev/xpfe/appshell/src/nsWebShellWindow.cpp:149
#15 0x41e64fbe in nsAppShellService::JustCreateTopWindow (this=<value optimized out>, aParent=0x446da520, aUrl=0x0, aChromeMask=4160750598, aInitialWidth=1, aInitialHeight=1, aIsHiddenWindow=false, aOpeningTab=0x0, aResult=0xbe83cb3c)
    at /home/steven/workspace/b2g/gecko-dev/xpfe/appshell/src/nsAppShellService.cpp:603
#16 0x41e6525a in nsAppShellService::CreateTopLevelWindow (this=0x44649e00, aParent=0x0, aUrl=0x0, aChromeMask=4160750598, aInitialWidth=-1, aInitialHeight=-1, aOpeningTab=0x0, aResult=0xbe83cb8c)
    at /home/steven/workspace/b2g/gecko-dev/xpfe/appshell/src/nsAppShellService.cpp:194
#17 0x41f2a362 in nsAppStartup::CreateChromeWindow2 (this=<value optimized out>, aParent=<value optimized out>, aChromeFlags=4160750598, aContextFlags=0, aURI=0x44610cc0, aOpeningTab=0x0, aCancel=0xbe83cdc8, _retval=0xbe83cdd8)
    at /home/steven/workspace/b2g/gecko-dev/toolkit/components/startup/nsAppStartup.cpp:666
#18 0x41e39cac in nsWindowWatcher::OpenWindowInternal (this=<value optimized out>, aParent=0x0, aUrl=<value optimized out>, aName=<value optimized out>, aFeatures=0x4464aca0 "centerscreen,chrome,modal,titlebar", aCalledFromJS=false, aDialog=true, aNavigate=true, 
    aOpeningTab=0x0, argv=0x4463fa80, _retval=0xbe83d0e0) at /home/steven/workspace/b2g/gecko-dev/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:723
#19 0x41e3a906 in nsWindowWatcher::OpenWindow (this=0x402deb80, aParent=0x0, aUrl=0x4464ac70 "chrome://global/content/commonDialog.xul", aName=0x40238dc8 "_blank", aFeatures=0x4464aca0 "centerscreen,chrome,modal,titlebar", aArguments=0x4464aac0, 
    _retval=0xbe83d0e0) at /home/steven/workspace/b2g/gecko-dev/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:347
#20 0x40c480f2 in NS_InvokeByIndex (that=0x402deb80, methodIndex=3, paramCount=<value optimized out>, params=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/xpcom/reflect/xptcall/md/unix/xptcinvoke_arm.cpp:164
#21 0x40fc771a in CallMethodHelper::Invoke (this=0xbe83d060) at /home/steven/workspace/b2g/gecko-dev/js/xpconnect/src/XPCWrappedNative.cpp:2375
#22 CallMethodHelper::Call (this=0xbe83d060) at /home/steven/workspace/b2g/gecko-dev/js/xpconnect/src/XPCWrappedNative.cpp:1736
#23 0x40fc843e in XPCWrappedNative::CallMethod (ccx=..., mode=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/js/xpconnect/src/XPCWrappedNative.cpp:1703
#24 0x40fc88c6 in XPC_WN_CallMethod (cx=0x40273100, argc=<value optimized out>, vp=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1288
#25 0x4256196c in js::CallJSNative (cx=0x40273100, native=0x40fc87ad <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/steven/workspace/b2g/gecko-dev/js/src/jscntxtinlines.h:231
#26 0x4259e03a in js::Invoke (cx=0x40273100, args=..., construct=js::NO_CONSTRUCT) at /home/steven/workspace/b2g/gecko-dev/js/src/vm/Interpreter.cpp:466
#27 0x42598948 in Interpret (cx=0x40273100, state=...) at /home/steven/workspace/b2g/gecko-dev/js/src/vm/Interpreter.cpp:2558
#28 0x4259c7ce in js::RunScript (cx=0x40273100, state=...) at /home/steven/workspace/b2g/gecko-dev/js/src/vm/Interpreter.cpp:413
#29 0x4259e08c in js::Invoke (cx=0x40273100, args=..., construct=js::NO_CONSTRUCT) at /home/steven/workspace/b2g/gecko-dev/js/src/vm/Interpreter.cpp:485
#30 0x4259eb04 in js::Invoke (cx=0x40273100, thisv=..., fval=..., argc=3, argv=0xbe83e1a8, rval=...) at /home/steven/workspace/b2g/gecko-dev/js/src/vm/Interpreter.cpp:522
#31 0x4245dc04 in JS_CallFunctionValue (cx=0x40273100, obj=<value optimized out>, fval=..., args=..., rval=...) at /home/steven/workspace/b2g/gecko-dev/js/src/jsapi.cpp:5000
#32 0x40fc5edc in nsXPCWrappedJSClass::CallMethod (this=0x4464a880, wrapper=<value optimized out>, methodIndex=<value optimized out>, info_=0x44577db8, nativeParams=0xbe83e4f0) at /home/steven/workspace/b2g/gecko-dev/js/xpconnect/src/XPCWrappedJSClass.cpp:1258
#33 0x40fb09b6 in nsXPCWrappedJS::CallMethod (this=0x4464b900, methodIndex=3, info=0x44577db8, params=0xbe83e4f0) at /home/steven/workspace/b2g/gecko-dev/js/xpconnect/src/XPCWrappedJS.cpp:520
#34 0x40c48ac0 in PrepareAndDispatch (self=<value optimized out>, methodIndex=<value optimized out>, args=0xbe83e5b4) at /home/steven/workspace/b2g/gecko-dev/xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp:93
#35 0x40c48108 in SharedStub () from /home/steven/workspace/b2g/gecko-dev/obj-emulator-debug/dist/bin/libxul.so
#36 0x41f0aad0 in ProfileMissingDialog (aNative=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/toolkit/xre/nsAppRunner.cpp:1821
#37 0x41f0cbcc in ProfileLockedDialog (aProfile=0x40201d90, aUnlocker=0x0, aNative=0x40204760, aResult=0xbe83ea34) at /home/steven/workspace/b2g/gecko-dev/toolkit/xre/nsAppRunner.cpp:1839
#38 0x41f0e714 in SelectProfile (this=0xbe83ea24, aExitFlag=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/toolkit/xre/nsAppRunner.cpp:2330
#39 XREMain::XRE_mainStartup (this=0xbe83ea24, aExitFlag=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/toolkit/xre/nsAppRunner.cpp:3642
#40 0x41f0ef54 in XREMain::XRE_main (this=0xbe83ea24, argc=<value optimized out>, argv=0xbe840c14, aAppData=0x23640) at /home/steven/workspace/b2g/gecko-dev/toolkit/xre/nsAppRunner.cpp:4069
#41 0x41f0f158 in XRE_main (argc=1, argv=0xbe840c14, aAppData=0x23640, aFlags=<value optimized out>) at /home/steven/workspace/b2g/gecko-dev/toolkit/xre/nsAppRunner.cpp:4298
#42 0x0000a240 in do_main (argc=1, argv=0xbe840c14) at /home/steven/workspace/b2g/gecko-dev/b2g/app/nsBrowserApp.cpp:163
#43 main (argc=1, argv=0xbe840c14) at /home/steven/workspace/b2g/gecko-dev/b2g/app/nsBrowserApp.cpp:256

[1] http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSwitch.cpp#474
Blocks: 964154
Hi Kersaw,

Could you check this bug? It causes debug emulator cannot boot.
Thanks.
Flags: needinfo?(kechang)
Sure, I will take a look.
Assignee: nobody → kechang
Hi Steven,

Could you explain more why there is a potential memory leakage problem?
I think sSwitchObserver should be only initialized once, right?

Thanks.
(In reply to StevenLee[:slee] from comment #1)
> Hi Kersaw,
> 
> Could you check this bug? It causes debug emulator cannot boot.
> Thanks.

Sure, I will take a look.
Flags: needinfo?(kechang)
Hi Steven,

Could you please test if the attached patch is working?
If yes, I will start the review process then.

Thanks.
Flags: needinfo?(slee)
(In reply to Kershaw Chang [:kershaw] from comment #5)
> Created attachment 8463882 [details] [diff] [review]
> Avoid calling RegisterUeventListener in main thread
> 
> Hi Steven,
> 
> Could you please test if the attached patch is working?
> If yes, I will start the review process then.
> 
> Thanks.
It works on my debug emulator. :)
Flags: needinfo?(slee)
Attachment #8463882 - Flags: review?(dhylands)
Hi Dave,

This bug is actually a regression from bug 964154.
Could you please help to review this patch?

Thanks.
Comment on attachment 8463882 [details] [diff] [review]
Avoid calling RegisterUeventListener in main thread

Review of attachment 8463882 [details] [diff] [review]:
-----------------------------------------------------------------

ok - I like this solution, but I think that it deserves mention in SwitchEventObserver::Init that it my get called on the main thread (and cite the place) as well as being called from the IOThread.

r=me with issues addressed.

::: hal/gonk/GonkSwitch.cpp
@@ +471,5 @@
>  
>  bool IsHeadphoneEventFromInputDev()
>  {
> +  if (sSwitchObserver) {
> +    return sSwitchObserver->GetHeadphonesFromInputDev();

If this function is being called from the main thread, then we shouldn't be looking at sSwitchObserver since its owned by the IOThread.

So we should only look at sSwitchObserver if this function is being called from the IOThread.

@@ +473,5 @@
>  {
> +  if (sSwitchObserver) {
> +    return sSwitchObserver->GetHeadphonesFromInputDev();
> +  }
> +  else {

nit: You returned, so remove the else and unindent the block.
Attachment #8463882 - Flags: review?(dhylands) → review+
Upload the final patch with the following changes:
1. Carry reviewer's name
2. Add some comments in SwitchEventObserver::Init to mention it might get called on main thread
3. Create new SwitchEventObserver in IsHeadphoneEventFromInputDev, since this is only called on main thread
Attachment #8463882 - Attachment is obsolete: true
Attachment #8464542 - Flags: review+
This landed under the wrong bug number. Please be more careful in the future.

https://hg.mozilla.org/mozilla-central/rev/c22459b75e0b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S1 (1aug)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: