773792 - gonk/GonkSensor.cpp declares global XPCOM objects

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Dave Hylands [:dhylands]]

I observed the warning on my b2g otoro phone (from a debug build):
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/work/B2G-otoro/gecko/xpcom/base/nsTraceRefcntImpl.cpp, line 141

I ran it under gdb and got the following backtrace:

#0  AssertActivityIsLegal () at /home/work/B2G-otoro/gecko/xpcom/base/nsTraceRefcntImpl.cpp:141
#1  0x411e3572 in NS_LogCtor_P (aPtr=0x0, aType=0x12820 "", aInstanceSize=32) at /home/work/B2G-otoro/gecko/xpcom/base/nsTraceRefcntImpl.cpp:1080
#2  0x41173712 in nsTArray_base (this=0x41dd6c70) at ../../dist/include/nsTArray-inl.h:14
#3  nsTArray (this=0x41dd6c70) at ../../dist/include/nsTArray.h:436
#4  InfallibleTArray (this=0x41dd6c70) at ../../dist/include/nsTArray.h:1282
#5  SensorData (this=0x41dd6c70) at /home/work/B2G-otoro/objdir-gecko/ipc/ipdl/PHal.cpp:158
#6  0x4117c756 in SensorStatus () at /home/work/B2G-otoro/gecko/hal/gonk/GonkSensor.cpp:153
#7  __static_initialization_and_destruction_0 () at /home/work/B2G-otoro/gecko/hal/gonk/GonkSensor.cpp:160
#8  global constructors keyed to GonkSensor.cpp () at /home/work/B2G-otoro/gecko/hal/gonk/GonkSensor.cpp:272
#9  0xb0001156 in call_array (ctor=0x41d9424c, count=75808, reverse=<value optimized out>) at bionic/linker/linker.c:1589
#10 0xb0001dc6 in call_constructors (si=0xb000b660, wr_offset=<value optimized out>) at bionic/linker/linker.c:1619
#11 __dl_$t (si=0xb000b660, wr_offset=<value optimized out>) at bionic/linker/linker.c:2013
#12 0xb00028a4 in init_library (name=<value optimized out>) at bionic/linker/linker.c:1169
#13 find_library (name=<value optimized out>) at bionic/linker/linker.c:1212
#14 0xb00031a2 in dlopen (filename=0xbe984ad8 "/system/b2g/libxul.so", flag=<value optimized out>) at bionic/linker/dlfcn.c:59
#15 0x00009abe in ReadDependentCB (aDependentLib=0x0, do_preload=<value optimized out>) at /home/work/B2G-otoro/gecko/xpcom/glue/standalone/nsGlueLinkingDlopen.cpp:191
#16 0x00009888 in XPCOMGlueLoadDependentLibs (xpcomDir=0xbe986b00 "/system/b2g", cb=0x9ab5 <ReadDependentCB>) at /home/work/B2G-otoro/gecko/xpcom/glue/standalone/nsXPCOMGlue.cpp:106
#17 0x00009a04 in XPCOMGlueLoad (xpcomFile=0xbe988b44 "/system/b2g/libxpcom.so", func=0xbe987b24) at /home/work/B2G-otoro/gecko/xpcom/glue/standalone/nsGlueLinkingDlopen.cpp:207
#18 0x000098ea in XPCOMGlueStartup (xpcomFile=0x0) at /home/work/B2G-otoro/gecko/xpcom/glue/standalone/nsXPCOMGlue.cpp:45
#19 0x0000892a in main (argc=1, argv=0xbe989c24) at /home/work/B2G-otoro/gecko/b2g/app/nsBrowserApp.cpp:191

GonkSensor.cpp line 160 corresponds to:
static SensorStatus sSensorStatus[NUM_SENSOR_TYPE];

Comment 1

[Justin Lebar (not reading bugmail)]

Without looking at the code, the solution may be to use ClearOnShutdown and Static{Ref,Auto}Ptr, from bug 772987.

Comment 2

[StevenLee[:slee]]

Hi Dave,

I will try Justin's method and see if it could work.

Comment 3

[StevenLee[:slee]]

(In reply to Justin Lebar [:jlebar] from comment #1)
> Without looking at the code, the solution may be to use ClearOnShutdown and
> Static{Ref,Auto}Ptr, from bug 772987.
Justin,
Thanks your suggestion.

Comment 4

[Dave Hylands [:dhylands]]

These particular messages occur at startup.

Comment 5

[Marco Chen [:mchen]]

This is the first bug I took and will discuss with Steven together.

Comment 6

[Marco Chen [:mchen]]

Hi all,

According to Jusin's suggestion, I tried to modify the static variable to StaticAutoPtr.
And when I want to find a way for initializing variables. There is no any function which should be guaranteed to call in the very first on GonkSensor.cpp even EnableSensorNotifications().

So do we need to encapsulate functions in GonkSensor.cpp to a GonkSensor class?
Then we can use the pattern of singleton to initialize static variables on getInstance().

Any suggestion? Thanks.

Comment 7

[Marco Chen [:mchen]]

If there is no necessary to do this on this stage, I will initialize them in EnableSensorNotifications() and followed by sSwitchThread.

Comment 8

[Justin Lebar (not reading bugmail)]

> So do we need to encapsulate functions in GonkSensor.cpp to a GonkSensor class?
> Then we can use the pattern of singleton to initialize static variables on getInstance().

This may be a sane thing to do (it would be easier to judge if you posted a WIP patch), but be careful if you take this approach: We've had bugs in the past when code did the following:

StaticRefPtr<GonkSensors> sGonkSensors;
already_AddRefed<GonkSensors> GonkSensors::GetInstance()
{
  if (!sGonkSensors) {
    sGonkSensors = new GonkSensors();
    ClearOnShutdown(&sGonkSensors);
  }
  
  nsRefPtr<GonkSensors> sensors = sGonkSensors;
  return sensors.forget();
}

some_function_called_after_xpcom_shutdown()
{
  GonkSensors::GetInstance()->ShutDown();
}

What happens is, we call GetInstance() in order to call ShutDown().  But at this point, ClearOnShutdown has already caused sGonkSensors to become null!  So the GetInstance() call ends up creating a new GonkSensors object, and then ClearOnShutdown asserts (because we're past shutdown), and...it's a mess.  :)

Comment 9

[Marco Chen [:mchen]]

Attachment: WIPv1 for review in advance of waiting try server account to test.

1. Following by Justin's suggestion to use StaticAutoPtr.
2, Following by comment 8, the StaticAutoPtr will be clear on ClearOnShutdown(). 
   So once someone call Enable/DisableSensorNotifications after that. The risk 
   will be occurred too.

Comment 10

[Justin Lebar (not reading bugmail)]

Please post diffs with 8 lines of context, by putting the following in your ~/.hgrc:

[defaults]
diff = -p -U 8
qdiff = -p -U 8
qnew = -U

See http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed for more info.

Review comments in a moment...

Comment 11

[Justin Lebar (not reading bugmail)]

Some style nits:

>+  if( !sSensorStatus[0]){

We always put a space after |if|, |for|, |while|, etc.  And we always put a space before same-line opening braces.  But we never put a space after an open parenthesis.  So

    if (!sSensorStatus[0]) {

>+    for(int i = 0; i < NUM_SENSOR_TYPE; i++){

space before {.

>+      sSensorStatus[i] = new SensorStatus();
>+      ClearOnShutdown(&sSensorStatus[i]);
>+    } 				

trailing whitespace on this line.

I think the change in this patch is basically right, but I'm confused about what the code you're changing is trying to do.  It seems like we should just eliminate the SensorStatus::data field altogether, since it's never used, at which point we have an array of ints (should be uint32_t), or an array of DebugOnly<uint32_t>'s.

I've asked for clarification in bug 734869 comment 22; you can cc yourself to that bug to follow along.  Unless you have some insight here?

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 657126 [details] [diff] [review]
WIPv1 for review in advance of waiting try server account to test.

f+ because, although I think this patch is basically right, we may be able to take a larger axe to this code.

Comment 13

[Marco Chen [:mchen]]

Hi Justin,

Thanks for your feedback.
Here is question for comment 10.
Before I submitted this patch I already do what you mentioned in comment 10.
So is an format wrong on my patch?

Comment 14

[Marco Chen [:mchen]]

For comment 11 about SensorStatus::data,

As I know Android::SensorService will record the last event on each sensor type.
Because once new one registers to listen a new sensor then this buffered event can be sent immediately when this sensor is not a continuous type of sensor.

So in the coding now, once we meet a non-continuous type of sensor the later registers can't listen the final event because the static environment condition didn't trigger sensor to output event again.

Comment 15

[Justin Lebar (not reading bugmail)]

> Before I submitted this patch I already do what you mentioned in comment 10.
> So is an format wrong on my patch?

Well, the patch has only 4 lines of context, so somehow those changes to your hgrc aren't making it into the patch.

> As I know Android::SensorService will record the last event on each sensor type.
> Because once new one registers to listen a new sensor then this buffered event can be 
> sent immediately when this sensor is not a continuous type of sensor.

Okay, but is it using the SensorStatus::data field?  hal seems to compile if I comment out that field, so I suspect not.

My point is just that I think we can remove that field, which would then obviate the need for ClearOnShutdown, because we'd have just an array of uint32_t's.

Comment 16

[Marco Chen [:mchen]]

> Okay, but is it using the SensorStatus::data field?  hal seems to compile if
> I comment out that field, so I suspect not.

I think the GonkSensor now didn't support what I mentioned for non-continous sensor. So I should implement it into next WIP version or it will be a potential bug in the future. Then that data field would be used. Wrap the GonkSensor.cpp into GonkSensor class with singleton pattern too.

> My point is just that I think we can remove that field, which would then
> obviate the need for ClearOnShutdown, because we'd have just an array of
> uint32_t's.

About the concerns on ClearOnShutdown, since this machenism is provided pepople have a way to release resources the ClearOnShutdown should guarantee it's safty not take care by each caller. Or the mess you mentioned will hapened again and again. 

So I suggest to create another bug to guarantee the ClearOnShutdown will do resource releasing in a safe timing (ex: the last one in sequence of shutdown event.) This maybe a long term solution.

Comment 17

[Justin Lebar (not reading bugmail)]

> So I suggest to create another bug to guarantee the ClearOnShutdown will do resource releasing in a 
> safe timing (ex: the last one in sequence of shutdown event.) This maybe a long term solution.

I'm happy to fix ClearOnShutdown to be safer, but it's not clear to me that we can do so safely.

The problem is that ClearOnShutdown may trigger an object's destructor.  And object destructors may run arbitrary code.  For example, one thing object destructors may do is call do_GetService(), which starts to fail (and return null) after some point during XPCOM shutdown.  If we were to call a destructor which doesn't check the return value of do_GetService() after the point during shutdown when do_GetService() starts to fail, then we'd crash on shutdown!

Right now, we clear ClearOnShutdown's pointers immediately before do_GetService() starts to return null (see xpcom/build/nsXPComInit.cpp, search for KillClearOnShutdown()), so if we moved it any later, we would risk hitting this issue.

So for this reason (and probably others), it's hard to say offhand that it would be safe to clear the smart pointers at a later point during shutdown.  We'd gain safety from one kind of bug at the expense of safety from another kind of bug.

> I think the GonkSensor now didn't support what I mentioned for non-continous sensor. So I should 
> implement it into next WIP version or it will be a potential bug in the future. Then that data field 
> would be used.

I /really/ don't like having code in the tree which isn't currently needed.

If and when we support non-continuous sensors in Gonk, we can have the data field, and that's fine.  Until then, we should not have that field, because it's not used for anything.

So I would like us to either remove the unnecessary field or use the field, but not write a patch which works around the unnecessary field.

If you want to to first add support for non-continuous sensors in Gonk (which you should do in a separate bug) and then fix this bug with the attached patch (or something similar), that's totally fine by me, assuming that we need non-continuous sensors in Gonk.

Comment 18

[Marco Chen [:mchen]]

Yes, totally agree your points on Clear of Code.

1. Thanks for your explanation on ClearOnShutdown.

2. For this bug only, the next patch will remove the data field for keeping code clear.

3. For non-continuous sensors, it will be provided in another bug.

Comment 19

[Marco Chen [:mchen]]

Attachment: Remove unused field of SensorStatus::data

Follow by Justin's comment about removing unused code.
Since SensorStatus::data was removed, there is no xpcom class on static variable.
Then this warning will be disappeared too.

Comment 20

[Marco Chen [:mchen]]

On the other hand, this patch keep the class SensorStatus to wrap the count variable.
So the extend variable in the future for each sensor can add into this class.
For example, the buffers for last events from each sensor.

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 657785 [details] [diff] [review]
Remove unused field of SensorStatus::data

Okay, but can we make SensorStatus an array of DebugOnly<uint32>'s instead of wrapping them inside the SensorStatus class?

Comment 22

[Marco Chen [:mchen]]

Attachment: keep count as native int array.

Follow Justin' comment to keep count as native int array.

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 657865 [details] [diff] [review]
keep count as native int array.

> +static DebugOnly<int> sRefCountOfSensor[NUM_SENSOR_TYPE] = {0};

Nit: I'd prefer sSensorRefCount.

More substantially: This happens to work, but if it works for the reason you intended, you're too clever for me.  :)

I had to look it up, but apparently if you do

  int arr[3] = {1};

then arr[0] == 1 and arr[1] == arr[2] == 0.

In the code from this patch, we're explicitly setting sRefCountOfSensor[0] to 0 and relying on the fact that all the other elements are /implicitly/ set to 0.

Instead, I'd prefer that we left off the |= {0}| and relied on the fact that static data is initialized to 0 by default.  (That's what sSwitchThread above is relying on.)

This is also good because it makes it less likely the compiler will add a static initializer for this data.  (We avoid static initializers because they slow startup.)  The static initializer for |= {0}| has no effect in release (or debug) builds, so the compiler should be smart enough to get rid of it, but in our experience, compilers are kind of dumb about this sometimes.  :(

Anyway, r=me if you change the name and get rid of the |= {0}|.  Please attach one final patch with these changes, following the instructions at [1].

If you don't have tryserver access yet, let me know and I can push your updated patch to try for you.

[1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 24

[Justin Lebar (not reading bugmail)]

> Please attach one final patch with these changes, following the instructions at [1].

...in particular, please fix the commit message to be more descriptive.  Instead of

> Bug-773792: Remove unused field and keep native int array

you could say

> Bug 773792: Make GonkSensor's sSensorStatus array an int[], fixing an instance of creating/destroying XPCOM objects from a static constructor/destructor. r=jlebar

Comment 25

[Mozilla RelEng Bot]

Try run for e3f64fa52b3e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e3f64fa52b3e
Results (out of 250 total builds):
    success: 230
    warnings: 17
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e3f64fa52b3e

Comment 26

[Mozilla RelEng Bot]

Try run for cf2d403660db is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cf2d403660db
Results (out of 191 total builds):
    success: 169
    warnings: 18
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cf2d403660db

Comment 27

[Marco Chen [:mchen]]

Attachment: Reviewed by Justin and be a candidate to do checkin-needed

Hi Justin,

I do the test on try server but it appeared some errors.
I checked that they are not related to my patch.
So do I need to check the patch as checkin-needed?

Thanks for the effort on my first bug.

Comment 28

[Justin Lebar (not reading bugmail)]

> I do the test on try server but it appeared some errors.
> I checked that they are not related to my patch.

The oranges (test failures) are all fine.

Normally I'd want to push again to make sure that the red (build failures) goes away when you pull from m-i and update your tree.  It looks like the build failure is coming from some unrelated code (you have to trudge through the log), but if your code /also/ triggered a build error, we wouldn't see it in the logs!

However, GonkSensor isn't compiled on Windows, so I think we're fine here.  :)  I'll go ahead and push this for you.

Comment 29

[Justin Lebar (not reading bugmail)]

Please note that I changed the beginning of the commit message from "Bug-773792" to "Bug 773792".  In the future, please omit this hyphen in your commit messages.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8638c60c914

The tree sheriffs will mark this bug as fixed once this code is merged to mozilla-central.

Congratulations on your first patch!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8638c60c914