1169674 - Cannot Raptor test multiple runs of the Camera app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[:Eli Perelman]

Somewhere on May 21st 2015, the Camera app stopped being able to handle 30 test runs using Raptor. In the Jenkins logs the application is able to prime correctly, launch the app once and get performance numbers, but always dies when it tries to do the second run.

I built B2G locally and tried this myself, and I can replicate the issue. The Camera will prime correctly, but once the app is launched for the first official run, a popup opens asking for location permissions. I believe it has always operated this way, but now the popup doesn't go away when we kill the Camera process.

I decided to capture the output of b2g-info when Raptor kills the Camera app after run 1:

           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS SWAP VSIZE OOM_ADJ USER
            b2g 3607    1   30.9    0 48.7 54.7 66.7  7.1 230.5       0 root
         (Nuwa) 3626 3607    1.2    0  0.3  3.0 10.7  5.8  95.6     -16 root
     Homescreen 3780 3626    8.2    0 14.7 19.8 31.2  4.8 134.0       2 u0_a3780
(Preallocated a 4321 3626    0.7    0  8.3 11.8 21.1  2.5 109.6       1 u0_a4321

As you can see, the Camera app is not running, but looking at the device at this moment, the location dialog is still visible. This is what causes Camera to fail, since Raptor cannot tap on the Camera app again on the homescreen with the dialog in the way.

I didn't see anything apparent in any of the Gaia commits around this timeframe, so maybe the problem is in Gecko. Regardless, here are the relevant commits as reported by Raptor:

Gaia last-known-good: ed3071e5cb7f1b9ef90b2e34e661d79adce86b30
Gecko last-known-good: 4ca15263c17f

Gaia first-known-bad: 108278984be8409d648d270bfc4758b84bc51bc4
Gecko first-known-bad: 6efd59656f9f

Comment 1

[:Eli Perelman]

Attachment: Jenkins logcat

FYI, this is the full logcat from the Jenkins test runs for ALL apps, so be sure to search for the relevant Camera sections if necessary.

Comment 2

[Gregor Wagner [:gwagner]]

Alive, do you think bug 1149003 could have caused this?
Eli, can you try a local run with this patch being backed out?

Comment 3

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=4ca15263c17f&tochange=6efd59656f9f

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

I don't think so. The permission dialog will be hidden when only
1. Switch app.
2. Gecko tells us to cancel as it told us to show by mozChromeEvent.

So I will say this might be gecko related. But I am open to the regression check result. If this is really caused by 1094759 I will fix it.

Comment 5

[Hema Koka [:hema]]

Who can help with the regression window to confirm the suspicion on bugs mentioned earlier?

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

QAnalysts is not set up to run Raptor test. However I can confirm this is reproducible manually, and confirmed this is a regression from Flame 2.2. I'll be working on getting the window.

Comment 7

[Pi Wei Cheng [:piwei] (inactive)]

mozilla-inbound regression window:

Last Working
Device: Flame
BuildID: 20150520170237
Gaia: b290c77ccb7ab0af599b3d8287b71b9970d8dcb0
Gecko: d98d6c08d2fd
Version: 41.0a1 (2.5 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

First Broken
Device: Flame
BuildID: 20150520171942
Gaia: b290c77ccb7ab0af599b3d8287b71b9970d8dcb0
Gecko: 810c13b0ac02
Version: 41.0a1 (2.5 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Same Gaia so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d98d6c08d2fd&tochange=810c13b0ac02

Possibly caused by changes made in Bug 873923 or Bug 1165162?

Comment 8

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Niel this seems to have been caused by the changes for Bug 873923.  Can you please take a look?

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QAnalysts were able to find a regression range.  Could you follow up with the dev of the patch please?

Comment 10

[Neil Deakin]

Bug 873923 wouldn't have affected this bug. Bug 1165162 could have done as it involves permissions, which are mentioned in comment 0.

Comment 11

[Hema Koka [:hema]]

(In reply to Neil Deakin from comment #10)
> Bug 873923 wouldn't have affected this bug. Bug 1165162 could have done as
> it involves permissions, which are mentioned in comment 0.

Bobby: could you please help with this? It looks like the regression range points to Bug 1165162?

Thanks
Hema

Comment 12

[Bobby Holley (:bholley)]

So, what is the normal mechanism that's supposed to prevent this dialog from appearing? Does the harness set the permissions, or is it using a pre-generated permissions database? That's probably where the trouble is.

Michael (CCed) is rebuilding the guts of the permission manager in bug 1165263, which may have an effect on this.

The platform behavior is changing here, and the test harness may need to adapt. The owner of the test harness should own this.

Comment 13

[:Eli Perelman]

Ah, so not a bug, it's a feature! :)

Since this is intended, I'll adapt our testing framework to try and work around this. Thanks!

Comment 14

[Bobby Holley (:bholley)]

(In reply to :Eli Perelman from comment #13)
> Ah, so not a bug, it's a feature! :)

Well, sorta - the issue is probably just that the way the test harness is setting the permissions to avoid this prompt isn't quite right and needs to be tweaked. If anything isn't behaving in the way you expect, please let me know. :-)

Comment 15

[:Eli Perelman]

Raptor doesn't do any real setup work for running its performance tests. It basically just taps on icons on the homescreen, waits for it to finish loading, kills the process, repeat.

Comment 16

[Bobby Holley (:bholley)]

Ok, then this is a bonafide change in behavior by the camera app? That's not expected, I don't think. Maybe we should flag the owner of the app to clarify what the expected behavior is.

Comment 17

[David Flanagan [:djf]]

I don't think the camera app has changed at all.  Sometime after it starts up, it asks to track the location, and this triggers the permission dialog. It used to be that the permission dialog would go away when raptor killed the app.

Now, when raptor kills the app, the permission dialog remains up, and raptor can't click on the homescreen anymore.  Or at least that's what it sounds like to me from reading through this.  

I think this is a real gecko bug, probably caused by one of the bugs mentioned in comment #7.

It might be easy to create a reduced test case that makes a geolocation request and then one or two seconds later calls window.close() to exit.  If the permission dialog remains up when that happens, it seems like a bug.

Eli: maybe you should re-open this?

Comment 18

[:Eli Perelman]

(In reply to David Flanagan [:djf] from comment #17)
> I don't think the camera app has changed at all.  Sometime after it starts
> up, it asks to track the location, and this triggers the permission dialog.
> It used to be that the permission dialog would go away when raptor killed
> the app.
> 
> Now, when raptor kills the app, the permission dialog remains up, and raptor
> can't click on the homescreen anymore.  Or at least that's what it sounds
> like to me from reading through this.  

Yes, that is exactly what is happening; I don't believe this to be a bug in Camera itself, rather the behavior of the dialog has changed.

> I think this is a real gecko bug, probably caused by one of the bugs
> mentioned in comment #7.
> 
> It might be easy to create a reduced test case that makes a geolocation
> request and then one or two seconds later calls window.close() to exit.  If
> the permission dialog remains up when that happens, it seems like a bug.
> 
> Eli: maybe you should re-open this?

I'm fine with that. Maybe something changed in System in how this dialog is generated?

Comment 19

[Bobby Holley (:bholley)]

This needs to be investigated by someone who knows how the permission dialogs work in Gaia.

Comment 20

[:Eli Perelman]

Gregor, is there anyone in System that can investigate this?

Comment 21

[Michael Henretty [:mikehenrty][:mhenretty]]

I'll investigate this.

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

The problem here is that the origin that comes through contains the appId, "app://camera.gaiamobile.org^appId=1005", and this does not match the origin of the app window (which does not contain the appId in the string.

Bobby, is this change in the origin expected? Do we need to start expecting appIds in origin strings that we get from Gecko? We can easily strip this out Gaia side, but there could be farther reaching ramifications here if the schema of origins is changing.

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Sorry, I realize that was not enough info. I was referring to the origin that comes through the 'permission-prompt' mozChromeEvent, that one has the appId. We compare this origin with the origin we have for the current mozApp, and if they match we close the dialog.

1.) http://hg.mozilla.org/mozilla-central/annotate/8bdd951da164/b2g/components/ContentPermissionPrompt.js#l342

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] mikehenrty:bug-1169674-strip-appid > mozilla-b2g:master

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Autolander from comment #24)
> Created attachment 8645161 [details] [review]
> [gaia] mikehenrty:bug-1169674-strip-appid > mozilla-b2g:master

This is a workaround, but let's see what bholley says before we go ahead with this approach.

Comment 26

[Bobby Holley (:bholley)]

We should definitely fix Gecko rather than Gaia here.

I think you want to pass principal.originNoSuffix instead of principal.origin here:

http://hg.mozilla.org/mozilla-central/annotate/8bdd951da164/b2g/components/ContentPermissionPrompt.js#l416

The basic idea is that principal.origin now contains all the information (including appId etc) that we use to determine whether two things are same-origin. principal.originNoSuffix returns what .origin returned before (and matches the concept of 'origin' that exists in HTML5).

We changed the default behavior because people often compare these strings to determine whether things are same-origin, so it's safest to return that information by default and then fix any consumers (like this) that are broken by it. So if you use .originNoSuffix, you want to make sure that the consumer isn't going to make any security decisions that would be wrong without taking appId, inBrowser, etc into account.

See the comment here: http://hg.mozilla.org/mozilla-central/file/461fc0a6a130/caps/nsIPrincipal.idl#l193

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [patch] use originNoSuffix

This seems to do the trip, as suggested.

Comment 28

[Michael Henretty [:mikehenrty][:mhenretty]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8390e0b3d18c

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8645729 [details] [diff] [review]
[patch] use originNoSuffix

Actually, Fabrice seems to be the owner of this file, so redirecting over to him.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8645729 [details] [diff] [review]
[patch] use originNoSuffix

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

::: b2g/components/ContentPermissionPrompt.js
@@ +412,5 @@
>      let details = {
>        type: type,
>        permissions: permissions,
>        id: requestId,
> +      origin: principal.originNoSuffix,

Please add a comment here indicating what this will be used for, and why the consumer doesn't need to worry about appId etc.

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8645729 [details] [diff] [review]
[patch] use originNoSuffix

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

r=me with bholley feedback addressed.

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [patch] use originNoSuffix w comments

Added the comment. r+ carries.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/f20381bf85d9

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 35

[Michael Henretty [:mikehenrty][:mhenretty]]

Eli, does this fix the raptor tests for camera?

Comment 36

[:Eli Perelman]

@mhenretty confirmed! Works great locally; I'll add it back to automation.