837953 - B2G crash @ nsContentPermissionRequestProxy::GetElement while launching Camera for perf tests

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

For the perf tests (https://datazilla.mozilla.org/b2g/) we launch each of several Gaia apps 30 times in a row to record their startup times.  However, we've had to disable the test for the Camera app because of frequent B2G crashes.

I was able to reproduce this locally using the perf test script, and when I did I submitted a crash report, which is here:  https://crash-stats.mozilla.com/report/index/9362218f-cf52-4728-a799-cd0f12130205

The perf test script simply launches the Camera app, then kills it, then waits 5 seconds, and repeats for 30 times.  The crash occurs after some random number of iterations, but we haven't seen all 30 iterations complete successfully in many runs.

Comment 1

[Jason Smith [:jsmith]]

This reads off as something Gregor would probably know how to fix.

I'm guessing were blowing up in the core permission prompt code.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Will consider an uplift nomination when one is available.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Lukas Blakk [:lsblakk] from comment #2)
> Will consider an uplift nomination when one is available.

I don't think I entirely agree on this one. We're trying to do perf analysis per a critical partner requirement to meet perf standards. If this blocks the ability to collect that data and analyze that, then I definitely think this blocks. And the bug reads off that way. Otherwise, the partner is just going through the build back up at us when we send it over them, they tell us the perf sucks, and we go back to being delayed for ship again.

jgriffin - Can you clarify? Are start-up perf tests for camera blocked by this bug?

Comment 4

[Jason Smith [:jsmith]]

Answered in IRC.

jsmith	jgriffin: About bug 837953 - is that going to block the ability to get effective start-up perf numbers for the camera app?
	bugbot	Bug https://bugzilla.mozilla.org/show_bug.cgi?id=837953 critical, --, ---, anygregor, NEW , B2G crash @ nsContentPermissionRequestProxy::GetElement while launching Camera for perf tests
	jgriffin	yes
	jgriffin	we've disabled the Camera test for this
	jsmith	jgriffin: Okay, then I pretty much disagree with triage on this. I'll go push back.
	jgriffin	thanks

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

What is the lowest number of times the test can run without crashing? Is there any way to separate the perf from the crash so that you get a baseline of perf time/number of successful runs?  The crash is something we want to see fixed and uplifted but in triage it was stated that the user is rarely going to reproduce this behaviour, hence not blocking.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Lukas Blakk [:lsblakk] from comment #5)
> What is the lowest number of times the test can run without crashing? Is
> there any way to separate the perf from the crash so that you get a baseline
> of perf time/number of successful runs?  The crash is something we want to
> see fixed and uplifted but in triage it was stated that the user is rarely
> going to reproduce this behaviour, hence not blocking.

Just a side comment - The nom isn't targeting a user scenario as being the case to block at all. The nom was for blocking the ability to get critical perf numbers that we need to know in order to know if we can ship or not with the camera app.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Lukas Blakk [:lsblakk] from comment #5)
> What is the lowest number of times the test can run without crashing? Is
> there any way to separate the perf from the crash so that you get a baseline
> of perf time/number of successful runs?  The crash is something we want to
> see fixed and uplifted but in triage it was stated that the user is rarely
> going to reproduce this behaviour, hence not blocking.

Also answered in IRC.

	jsmith	jgriffin: What is the minimum amount of times the camera start-up test can be ran before it crashes?
	jgriffin	it's quite random, sometime it crashes on the 2nd or 3rd iteration, sometimes on the 25th

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Given comment 7 and that this can crash in automation (let alone for a user) on the 2nd or 3rd time and considering that gathering this perf data is a partner need, I'm changing it to blocking.

Comment 9

[Gregor Wagner [:gwagner]]

jgriffin: Just tried to reproduce it locally but not very successful.

So this happens on an unagi? How do you drive the performance tests? The camera app shows a permission prompt, can you tell me if you click allow, deny or of you just leave it and kill the app?

Maybe it's some strange behavior when we leave the permission prompt and kill the app in the meantime or if we start the app again and we get 2 permission dialogs...

Comment 10

[Jonathan Griffin (:jgriffin)]

Yes, this happens on an unagi.  We are using freshly-flashed phones and we're just leaving the permissions prompt without clicking it.

To drive the tests yourself, you need to use an unagi_eng build (not a user) build.  Then:

1 - hg clone http://hg.mozilla.org/users/tmielczarek_mozilla.com/b2gperf
2 - cd b2gperf
3 - virtualenv --no-site-packages venv
4 - source venv/bin/activate
5 - python setup.py install
6 - b2gperf --delay 5 venv/lib/python2.7/site-packages/gaiatest-0.5-py2.7.egg/gaiatest/atoms Camera

The path to 'atoms' in 6) may be slightly different depending on your OS and Python version.  If you have trouble locating the 'atoms' dir, you can 

  git clone git://github.com/mozilla/gaia-ui-tests.git

and then point it at the gaiatest/atoms directory within the new clone.

Comment 11

[Jonathan Griffin (:jgriffin)]

If you want to use the same build as we have been, it's at https://releases.mozilla.com/b2g/latest/unagi_eng.zip

Comment 12

[Jonathan Griffin (:jgriffin)]

Using the above build and script, I just reproduced the crash on iteration 23.

Comment 13

[Gregor Wagner [:gwagner]]

I am trying to reproduce it with my build. Just seeing how it crashes doesn't help me much. I need to catch it in GDB ideally with a non-optimized version that includes all the symbols.

Comment 14

[Gregor Wagner [:gwagner]]

Got it in GDB. mParent->mElement is null

Program received signal SIGSEGV, Segmentation fault.
0x41198a70 in nsContentPermissionRequestProxy::GetElement (this=0x48278070, aRequestingElement=0xbeeddb70) at /Volumes/2mac/gaia/b2g18/dom/base/nsContentPermissionHelper.cpp:95
95	  NS_ADDREF(*aRequestingElement = mParent->mElement);
(gdb) p mParent
$1 = (class mozilla::dom::ContentPermissionRequestParent *) 0x48c92580
(gdb) p aRequestingElement
$2 = (class nsIDOMElement **) 0xbeeddb70
(gdb) p *aRequestingElement
$3 = (class nsIDOMElement *) 0x0
(gdb) p mParent
$4 = (class mozilla::dom::ContentPermissionRequestParent *) 0x48c92580
(gdb) p *mParent
$5 = {<mozilla::dom::PContentPermissionRequestParent> = {<mozilla::ipc::RPCChannel::RPCListener> = {<mozilla::ipc::SyncChannel::SyncListener> = {<mozilla::ipc::AsyncChannel::AsyncListener> = {<mozilla::ipc::HasResultCodes> = {<No data fields>}, 
          _vptr.AsyncListener = 0x42cde8d8}, <No data fields>}, <No data fields>}, <mozilla::ipc::IProtocolManager<mozilla::ipc::RPCChannel::RPCListener>> = {
      _vptr.IProtocolManager = 0x42cde960}, mChannel = 0x4aefa428, mManager = 0x452ec744, mId = -21, mState = mozilla::dom::PContentPermissionRequest::__Null}, 
  mPrincipal = {<nsCOMPtr_base> = {mRawPtr = 0x472d6e60}, <No data fields>}, mElement = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, mProxy = {<nsCOMPtr_base> = {
      mRawPtr = 0x48278070}, <No data fields>}, mType = {<nsACString_internal> = {mData = 0x472d6e48 "geolocation", mLength = 11, mFlags = 5}, <No data fields>}, 
  mAccess = {<nsACString_internal> = {mData = 0x45b1e1d8 "unused", mLength = 6, mFlags = 5}, <No data fields>}}
(gdb) p this
$6 = (nsContentPermissionRequestProxy * const) 0x48278070
(gdb) p *this
$7 = {<nsIContentPermissionRequest> = {<nsISupports> = {_vptr.nsISupports = 0x42cde8a0}, <No data fields>}, mRefCnt = {mValue = 3}, mParent = 0x48c92580, mType = {<nsACString_internal> = {
      mData = 0x472d6e48 "geolocation", mLength = 11, mFlags = 5}, <No data fields>}, mAccess = {<nsACString_internal> = {mData = 0x45b1e1d8 "unused", mLength = 6, 
      mFlags = 5}, <No data fields>}}
(gdb) l
90	  NS_ENSURE_ARG_POINTER(aRequestingElement);
91	  if (mParent == nullptr) {
92	    return NS_ERROR_FAILURE;
93	  }
94	
95	  NS_ADDREF(*aRequestingElement = mParent->mElement);
96	  return NS_OK;
97	}
98	
99	NS_IMETHODIMP
(gdb) p *mParent
$8 = {<mozilla::dom::PContentPermissionRequestParent> = {<mozilla::ipc::RPCChannel::RPCListener> = {<mozilla::ipc::SyncChannel::SyncListener> = {<mozilla::ipc::AsyncChannel::AsyncListener> = {<mozilla::ipc::HasResultCodes> = {<No data fields>}, 
          _vptr.AsyncListener = 0x42cde8d8}, <No data fields>}, <No data fields>}, <mozilla::ipc::IProtocolManager<mozilla::ipc::RPCChannel::RPCListener>> = {
      _vptr.IProtocolManager = 0x42cde960}, mChannel = 0x4aefa428, mManager = 0x452ec744, mId = -21, mState = mozilla::dom::PContentPermissionRequest::__Null}, 
  mPrincipal = {<nsCOMPtr_base> = {mRawPtr = 0x472d6e60}, <No data fields>}, mElement = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, mProxy = {<nsCOMPtr_base> = {
      mRawPtr = 0x48278070}, <No data fields>}, mType = {<nsACString_internal> = {mData = 0x472d6e48 "geolocation", mLength = 11, mFlags = 5}, <No data fields>}, 
  mAccess = {<nsACString_internal> = {mData = 0x45b1e1d8 "unused", mLength = 6, mFlags = 5}, <No data fields>}}
(gdb)

Comment 15

[Gregor Wagner [:gwagner]]

Dougt: Should we check for mFrameElement and return null if not set?
https://mxr.mozilla.org/mozilla-b2g18/source/dom/ipc/TabParent.cpp#363

Comment 16

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 17

[Doug Turner (:dougt)]

mFrameElement can be null.  

Maybe just changing:

 https://mxr.mozilla.org/mozilla-b2g18/source/dom/base/nsContentPermissionHelper.cpp#95

to  NS_IF_ADDREF(*aRequestingElement = mParent->mElement);

Is the way to go.

Comment 18

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 19

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8094f5ab563c

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8094f5ab563c

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/58fc2b868885
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/81a4937469b6

Comment 22

[Jason Smith [:jsmith]]

Jonathan - Can you verify that this patch allows you to run the camera start-up perf tests multiple times without crashing?

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Yes, the camera app now launches successfully for the performance tests.

Comment 24

[Jason Smith [:jsmith]]

Marking verified per comment 23