824177 - mozbrowser frames inside gaia browser app are popping up with NO_APP_ID

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P1)

Description

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

Attachment: logcat

## Environment :
Unagi phone, build 2012-12-21

## Repro :
1. go to KTUL.com

## Expected :
1. you can see the site

## Actual :
1. Well this is embarassing ...

## Note :
1. logcat attached
2. I didn't see any sigkills...

Comment 1

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

Attachment: screenshot

Comment 2

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

The same thing happened for Kron4.com, except with Kron4.com, you had to try going to a newslink, and when that happened there was an attempt for a sigkill : 

12-21 17:23:37.538: D/memalloc(107): /dev/pmem: Freeing buffer base:0x4b8aa000 size:184320 offset:4521984 fd:187
12-21 17:23:37.538: D/memalloc(107): /dev/pmem: Freeing buffer base:0x4b8d7000 size:184320 offset:4706304 fd:192
12-21 17:23:37.819: I/Gecko(107): [Parent 107] WARNING: waitpid failed pid:589 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 260
12-21 17:23:38.399: E/profiler(674): Registering start signal
12-21 17:23:38.399: E/profiler(673): Registering start signal
12-21 17:23:39.400: E/GeckoConsole(673): [JavaScript Warning: "Unknown property '-moz-align-self'.  Declaration dropped." {file: "resource://gre-resources/ua.css" line: 44}]
12-21 17:23:39.410: E/GeckoConsole(674): [JavaScript Warning: "Unknown property '-moz-align-self'.  Declaration dropped." {file: "resource://gre-resources/ua.css" line: 44}]
12-21 17:23:39.821: I/Gecko(107): [Parent 107] WARNING: waitpid failed pid:589 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 260
12-21 17:23:39.821: I/Gecko(107): [Parent 107] WARNING: Failed to deliver SIGKILL to 589!(3).: file ../../../gecko/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118
12-21 17:23:39.901: I/Gecko(673): ###################################### forms.js loaded
12-21 17:23:39.921: I/Gecko(674): ###################################### forms.js loaded
12-21 17:24:46.015: I/IdleService(107): Remove idle observer 438d84f0 (1 seconds)
12-21 17:24:46.225: D/memalloc(107): /dev/pmem: Freeing buffer base:0x4b752000 size:614400 offset:3112960 fd:165
12-21 17:24:46.225: D/memalloc(107): /dev/pmem: Freeing buffer base:0x4b904000 size:614400 offset:4890624 fd:110
12-21 17:24:46.306: E/GeckoConsole(107): [JavaScript Error: "Image corrupt or truncated: app://system.gaiamobile.org/style/lockscreen/images/mask.png" {file: "app://system.gaiamobile.org/style/lockscreen/images/mask.png" line: 0}]
12-21 17:28:36.090: I/IdleService(107): Get idle time: time since reset 300010 msec

Might have to log this separately?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For the kron4 test case,

I/Gecko   (11101): [Parent 11101] WARNING: NeckoParent::AllocPHttpChannel: FATAL error: TabParent reports appId=NECKO_NO_APP_ID but is a mozbrowser: KILLING CHILD PROCESS

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That's bad irrespective of the necko stuff.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Blocks a blocker.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Blocks two blockers, because it means clear-private-data is broken too.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, typo.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Opened windows of browsers inherit their containing app

Let me know the best way to write a test for this.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 695105 [details] [diff] [review]
Opened windows of browsers inherit their containing app

Did you mean to assign the bug to me?  I can take it if you want, but I probably won't be able to get to it until Jan 7 or so.

>+        mContainingAppId = context->BrowserOwnerAppId();

I don't think this is quite right.

ipcContext.isBrowserElement() tells us whether we're a browser.  But it doesn't tell us whether our opener is a browser.

But context->BrowserOwnerAppId() gets our opener's app-id only if the opener is a browser.  If context (our opener) is not a browser, BrowserOpenerAppId() returns NO_APP.

I /think/ it should be mContainingAppId = context->OwnOrContainingAppId().  If the opener is an app, that's the app's id, and if the opener is a browser, that's the browser's owner id.

As far as writing a test for this, you can of course create a mozbrowser which calls window.open and do that business.  It's not clear to me what else needs to be done to trigger this bug, though.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Opened windows of browsers inherit their containing app, v2

jduell, in which testing configurations do we have necko security enabled?  I'm not sure which harness will actually test this correctly.

The test just needs to do the following
 - create a <browser> inside an <app>
 - have the <browser> window.open() another window
 - load any HTML file in that new window

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 695542 [details] [diff] [review]
Opened windows of browsers inherit their containing app, v2

r=me, but I would <3 a test.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I would too.  Maybe jdm knows the answer to comment 10?

Comment 13

[Josh Matthews [:jdm]]

I don't know what kind of testing abilities we have for apps. I am only aware of the test harnesses in which we have disabled the checks, being xpcshell and the dom/browser-element tests.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The dom/browser-element tests are exactly what I need to write this test :/.  Is there a bug to re-enable checking there?  Is it kosher to turn checking on/off willy nilly?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I guess an alternative is introspecting on the containing app ID from chrome script.  Is there a way I can do that?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Cowardly punting to a followup because this looks really hard.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 18

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b16ea3a8a4d2
https://hg.mozilla.org/releases/mozilla-aurora/rev/e7a75395decc

Comment 19

[Graeme McCutcheon [:graememcc]]

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

Comment 20

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> I guess an alternative is introspecting on the containing app ID from chrome
> script.  Is there a way I can do that?

I don't get what the problem is -- content can get its app manifest, and I think with a little special powers magic you can get the app ID from a mochitest.  IIRC we even have code in the browser-element tests which does this.

Such a test would work on desktop as well as b2g.

> I am only aware of the test harnesses in which we have disabled the checks, being 
> xpcshell and the dom/browser-element tests.

I don't remember being told that we disabled these tests on B2G, but I could be forgetting.  Could you please point me to the bug?

> Is it kosher to turn checking on/off willy nilly?

Certainly not without asking the owner of those tests, I should think.

Comment 21

[Josh Matthews [:jdm]]

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #20)
> > I am only aware of the test harnesses in which we have disabled the checks, being 
> > xpcshell and the dom/browser-element tests.
> 
> I don't remember being told that we disabled these tests on B2G, but I could
> be forgetting.  Could you please point me to the bug?
> 
> > Is it kosher to turn checking on/off willy nilly?
> 
> Certainly not without asking the owner of those tests, I should think.

Bug 782542 comment 47. Sorry for not roping you in.

Comment 22

[Justin Lebar (not reading bugmail)]

I'm cc'ed on that bug, actually.  But I'm confused: jduell is saying in there that we don't use browser-not-in-app in production, which is true.  But I thought we were saying here that we aren't running dom/browser-element tests on B2G?

Sorry if I'm misunderstanding.

Comment 23

[Josh Matthews [:jdm]]

I only said that we disable the necko security checks when running the dom/browser-element tests, due to the unusual situation of browser without app. The tests themselves still run as usual.

Comment 24

[Justin Lebar (not reading bugmail)]

Ooooh, okay.  I understand now.  Sorry for the confusion!