Closed Bug 851214 Opened 9 years ago Closed 6 years ago

Allow OOP mozbrowser w/o enclosing app

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: kgrandon, Assigned: jduell.mcbugs)

Details

(Whiteboard: [desktop only][necko-would-take])

Attachments

(1 file, 1 obsolete file)

Granting the browser permission in Firefox nightly doesn't seem to work. I'm seeing an error like: loading http://yahoo.com/, 1
NeckoParent::AllocPHttpChannel: FATAL error: TabParent reports appId=NECKO_NO_APP_ID but is a mozbrowser: KILLING CHILD PROCESS

###!!! [Parent][AsyncChannel] Error: Value error: message was deserialized, but contained an illegal value


###!!! [Parent][AsyncChannel] Error: Route error: message sent to unknown actor ID

[Parent 76951] WARNING: waitpid failed pid:76959 errno:10: file ../../../../ipc/chromium/src/base/process_util_posix.cc, line 260

###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv

[Parent 76951] WARNING: waitpid failed pid:76959 errno:10: file ../../../../ipc/chromium/src/base/process_util_posix.cc, line 260
[Parent 76951] WARNING: waitpid failed pid:76959 errno:10: file ../../../../ipc/chromium/src/base/process_util_posix.cc, line 260
[Parent 76951] WARNING: Failed to deliver SIGKILL to 76959!(3).: file ../../../../ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118
[Parent 76951] WARNING: waitpid failed pid:76959 errno:10: file ../../../../ipc/chromium/src/base/process_util_posix.cc, line 260
[Parent 76951] WARNING: Failed to deliver SIGKILL to 76959!(3).: file ../../../../ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118
As it's not possible to get access to privileged APIs by installing an app in Firefox, you should point out that you're adding the browser permission by calling the permission manager directly from Chrome, using a plugin :)

https://github.com/KevinGrandon/Firefox-OS-Runtime/blob/master/lib/main.js#L13
So is this desktop only then? Or is b2g generally affected?
@Ben - Yes, that's a good idea :) You can also run this in the browser script console, the command would look something like:

host = 'http://browser.gaiamobile.org:8080';
perm = Components.classes["@mozilla.org/permissionmanager;1"]
                 .createInstance(Components.interfaces.nsIPermissionManager);
ios = Components.classes["@mozilla.org/network/io-service;1"]
                .getService(Components.interfaces.nsIIOService);
uri = ios.newURI(host, null, null);
perm.add(uri, 'browser', 1);
(In reply to Jason Smith [:jsmith] from comment #2)
> So is this desktop only then? Or is b2g generally affected?

This is desktop, and purely for developer testing right now. I think that QA could largely ignore this one for now :)
(In reply to Kevin Grandon :kgrandon from comment #4)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > So is this desktop only then? Or is b2g generally affected?
> 
> This is desktop, and purely for developer testing right now. I think that QA
> could largely ignore this one for now :)

Okay, good. Don't need to panic that something didn't regress then. I'm sticking a small whiteboard in just as a note.
Whiteboard: [desktop only]
> NeckoParent::AllocPHttpChannel: FATAL error: TabParent reports appId=NECKO_NO_APP_ID but 
> is a mozbrowser: KILLING CHILD PROCESS

I think this is saying that you're trying to create an OOP mozbrowser that's outside an app.  Necko is not allowing that: I think it's saying that any OOP mozbrowser must be inside an app.

I'm kind of surprised that this fails, because the tests do exactly this.  So maybe I'm missing something.  Maybe jduell knows better.
Hi Jason - any ideas here? Thanks!
Flags: needinfo?(jduell.mcbugs)
I was told (by jlebar in fact! :) that we didn't use this model yet (OOP browser w/o an app frame above it), so I disabled it just in case any of the {cookie,[app]cache,auth}-jar code is written w/o handling the {appID=0, inBrowser=true} case.  It sounds like you need that to work now?
Flags: needinfo?(jduell.mcbugs)
Specifically I'm worried about the case of code doing premature shortcuts like

   if (appID == NO_APP_ID /*aka 0*/)
     // do something that also assumes inBrowser=false

I'm not sure all the *-jar code was written to assume that we could have appid=0 but inbrowser=true.

I just checked the cookie-jar code and it seems fine.  If we can also verify that auth, http cache, appcache, and anything else that uses jars (local storage? others?) I can write a patch that disables the check.   How badly do we need this BTW?
moving to networking since that's where the code is that needs to be changed.
Component: General → Networking
Product: Boot2Gecko → Core
Summary: Browser permission doesn't work in FF nightly → Allow OOP mozbrowser w/o enclosing app
Attached patch Allow appID=0, inBrowser=true (obsolete) — Splinter Review
I'd like both honza and jdm to review this just to confirm that all the relevant *-jars handle this case correctly.
Assignee: nobody → jduell.mcbugs
Status: NEW → ASSIGNED
Attachment #726234 - Flags: review?(honzab.moz)
Attachment #726234 - Flags: review?(josh)
OS: Mac OS X → All
Hardware: x86 → All
Thank you for the quick patch Jason - it's much appreciated. It's not super urgent, but does allow us to have faster development cycles if we can test this stuff in the browser.
Comment on attachment 726234 [details] [diff] [review]
Allow appID=0, inBrowser=true

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

In DOMStorage and Offline App Cache I can see the code is bypassing addition of a jar id when:

(!isInBrowserElement && appId == NECKO_NO_APP_ID)

So, I think we are safe.
Attachment #726234 - Flags: review?(honzab.moz) → review+
> I was told (by jlebar in fact! :) that we didn't use this model yet (OOP browser w/o an 
> app frame above it)

I thought we established that we don't use this /in production/?  What's weird is that the existing mozbrowser tests certainly use this setup, and they seem to work just fine.
jlebar: Which existing mozbrowser tests?  Are you sure we didn't disable them, or make them run in-process?
Flags: needinfo?(justin.lebar+bug)
I'm pretty sure we disabled the necko security checks for the mozbrowser tests.
(In reply to Jason Duell (:jduell) from comment #15)
> jlebar: Which existing mozbrowser tests?  Are you sure we didn't disable
> them, or make them run in-process?

dom/browser-element/mochitest.

I sure hope we haven't disabled them or made them all run in-process!
Flags: needinfo?(justin.lebar+bug)
(In reply to Josh Matthews [:jdm] from comment #16)
> I'm pretty sure we disabled the necko security checks for the mozbrowser
> tests.

So they can just set the network.disable.ipc.security pref to true?
Guys, this is awesome. Disabling that preference totally fixes our development cycle - and it appears it's something I can implement within the dev environment as well.

Thank you for the investigation on this! As we're already using 'black magic' for the browser permission, I suppose we could also use it for this preference for now. If this is not something we want to change, please feel free to close this as wontfix.
> feel free to close this as wontfix

Now that we've audited cookies/AppCache/DomStorage, we might as well verify/fix the rest of the *-jar code (auth and HTTP cache, anything else?) and land this.  So leaving r?jdm.
Oh, and we'll need to write another patch to remove the network.disable.ipc.security pref tweaks in the tests if we land the patch under review here.  Cruft is bad.
Comment on attachment 726234 [details] [diff] [review]
Allow appID=0, inBrowser=true

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

Verified the HTTP cache, HTTP auth cache, and wyciwyg protocol.
Attachment #726234 - Flags: review?(josh) → review+
:jduell, I know this is nearly 3 years ago now, but I thought you might be able to help here.

I am doing a code audit around enabling <iframe mozbrowser> for desktop (bug 1238160).  As part of my audit, I stumbled on this bug and noticed that the r+ed patch in this bug landed[1] on 2013-04-16 and then was backed out[2] the same day.

Is the change in this patch still believed to be correct?  Should it land for real?  At the moment, it does not really matter on desktop since Necko security is disabled there, but perhaps landing this patch would allow it to turn on in the future, especially with the <iframe mozbrowser> case I am investigating.

[1]: https://hg.mozilla.org/mozilla-central/rev/bfd7c7290cab
[2]: https://hg.mozilla.org/mozilla-central/rev/665d1b10c562
Flags: needinfo?(jduell.mcbugs)
Honza, would this patch still work?  I'm not sure, since we switched over most things to use origin, etc.

ryans: necko security is disabled on desktop?  Can you point me at where that decision was made?  It's a reasonably large security hole (from a sandbox perspective at least) so it would be nice to fix that.  (Maybe it doesn't matter since we're not using apps a lot, but I'd hate to see the checks just rot).
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
Flags: needinfo?(jryans)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #25)
> ryans: necko security is disabled on desktop?  Can you point me at where
> that decision was made?  It's a reasonably large security hole (from a
> sandbox perspective at least) so it would be nice to fix that.  (Maybe it
> doesn't matter since we're not using apps a lot, but I'd hate to see the
> checks just rot).

It is currently disabled[1] with the comment:

"Necko IPC security checks only needed for app isolation for cookies/cache/etc:
currently irrelevant for desktop e10s"

This happened in bug 891954, which landed[2] on 2013-07-27.

[1]: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/app/profile/firefox.js#1443-1445
[2]: https://hg.mozilla.org/mozilla-central/rev/44b9c2407781
Flags: needinfo?(jryans)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #25)
> Honza, would this patch still work?  I'm not sure, since we switched over
> most things to use origin, etc.

OK to go for me.
Flags: needinfo?(honzab.moz)
:jduell, should we proceed with landing it given comment 27?
Flags: needinfo?(jduell.mcbugs)
Yes, let's land this then.
Keywords: checkin-needed
Flags: needinfo?(jduell.mcbugs)
has problems to apply:

applying 851214.allow.oop.appid0
patching file netwerk/ipc/NeckoParent.cpp
Hunk #1 FAILED at 92
1 out of 1 hunks FAILED -- saving rejects to file netwerk/ipc/NeckoParent.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 851214.allow.oop.appid0
Flags: needinfo?(jduell.mcbugs)
Keywords: checkin-needed
Bill: this patch bitrotted.  We used to throw an error for appID=0, inBrowser=true, now we'll allow it (unless HasOwnApp() returns true, in which case things seem inconsistent).  Does that seem right to you?
Attachment #726234 - Attachment is obsolete: true
Flags: needinfo?(jduell.mcbugs)
Attachment #8721099 - Flags: review?(wmccloskey)
Whiteboard: [desktop only] → [desktop only][necko-would-take]
Comment on attachment 8721099 [details] [diff] [review]
V2: Allow appID=0, inBrowser=true

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

This looks good to me. It seems like the origin attributes stuff answered the concern about appId==0 checks?

::: netwerk/ipc/NeckoParent.cpp
@@ +133,5 @@
>      if (appId == NECKO_UNKNOWN_APP_ID) {
>        continue;
>      }
>      // We may get appID=NO_APP if child frame is neither a browser nor an app
> +    if (appId == NECKO_NO_APP_ID && tabContext.HasOwnApp() {

You need a paren at the end.
Attachment #8721099 - Flags: review?(wmccloskey) → review+
:jduell, is this ready to land?
Flags: needinfo?(jduell.mcbugs)
Ship it!
Flags: needinfo?(jduell.mcbugs)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d72759589f6f
https://hg.mozilla.org/mozilla-central/rev/55fb703a89e5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.