Closed Bug 802366 Opened 12 years ago Closed 12 years ago

Make the browser vs. app distinction clear, make our APIs sane, and let browser processes inherit their parents' app-ids

Categories

(Core :: DOM: Navigation, defect, P1)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bent.mozilla, Assigned: justin.lebar+bug)

References

(Depends on 1 open bug)

Details

Attachments

(14 files, 1 obsolete file)

Prelude, part 1: Rename in-process-browser-frame-shown to in-process-browser-or-app-frame-shown.
4.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Prelude, part 2: Rename GetParentIgnoreBrowserFrame to GetSameTypeParentIgnoreBrowserAndAppBoundaries.
4.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Prelude, part 3: Make some methods on nsIPrincipal infallible, and improve documentation on other methods.
5.97 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Prelude, part 4: Use and simplify nsScriptSecurityManager::GetDocShellCodebasePrincipal.
2.74 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Prelude, part 5: Improve comments in nsIDocShellTreeItem.idl and nsILoadContext.idl.
2.12 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Part 1 (fold into main part): s/GetIsContentBoundary/GetIsBrowserOrApp/ s/GetIsBelowContentBoundary/GetIsInBrowserOrApp/.
12.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Part 2 (fold into main part): Changes to nsIMozBrowserFrame (ReallyIsBrowser --> ReallyIsBrowserOrApp) and users of the interface.
9.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Part 3a, the main event: docshell changes.
17.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Part 3b, the main event: frameloader changes.
12.96 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 802366 - Part 3c, the main event: dom/ipc changes.
34.13 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Epilogue: Remove workaround for bug 802366 in Webapps.jsm.
977 bytes, patch
sicking
: review+
Details | Diff | Splinter Review
Volume II part 1: Add and use TabContext, which holds app-in-app data.
59.34 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Volume II, Part 2: Use TabContext to make IDB handling more correct in TabParent.
5.45 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Volume II, part 2a: Followup.
4.42 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review 
Spun off from bug 786295, we need to fix our code so that browsers within apps report the correct appId and browser flag. Currently IndexedDB sees all browsers within apps as having appId 0.
Attached patch WIP (obsolete) — — Splinter Review 
This is the beginning of what we've got so far. Bug 786295 comment 33 has not yet been addressed.
blocking-basecamp: --- → ?
This is breaking our security model so this is definitely a blocker.

Justin: Do you think you could finish this one up? This stuff feels complex enough that it needs your help.
Assignee: nobody → justin.lebar+bug
blocking-basecamp: ? → +
Priority: -- → P1
It's going to take time away from memshrink bugs if I work on this...  Is Ben giving up, or does he have something more important than memshrink to be doing?
Basically it seemed like a poor use of resources given the amount of guidance he needed from you.
(In reply to Jonas Sicking (:sicking) from comment #4)
> Basically it seemed like a poor use of resources given the amount of
> guidance he needed from you.

Can we be explicit about the trade-off here?  What is Ben going to work on instead?  Can he pick up MemShrink bugs?

It's obviously helpful for a capable programmer like Ben to learn this code.
Even if we agreed that memshrink was the absolute top priority project right now that doesn't mean that we can only do memshrink in order to ship B2G. So wasting resources is a bad idea no matter what.

Bent is going to be working on testing our IPC code since we have very little tests for it and B2G is basically the first time that use it to the extent that we are. My impression was that we're seeing a lot of system-wide issues due to low memory right now, even though in theory we should be able to kill child processes on OOM and move on unaffected.
I guess I don't buy the idea that teaching Ben about this code is somehow a waste of resources.

But it sounds like he doesn't want to work on it anymore, in which case of course continuing to teach him about it would be a waste.  That's OK.  But unfortunately it means that we're going to have to trade off between two high priorities, because I'm already working well above capacity.
Well, the question was if it was actually saving you any work given the amount of time needed to be spent on reviews. If it was then I can try to find someone else to finish this up.
If I have to get someone else up to the place where Ben was, that's probably not going to save me any work.
Ben, can you please give me a precise STR for the behavior you need fixed here?

If you've written any automated testcases, or if testcases you've written can be easily modified to test this behavior, that would also be useful.
Flags: needinfo?(bent.mozilla)
(In reply to Justin Lebar [:jlebar] from comment #10)
> Ben, can you please give me a precise STR for the behavior you need fixed
> here?

Ben, I could really use some help here, so I know how to test the changes I'm making in this bug.
Sorry, I've been distracted trying to finish/land bug 786295.

So here's the deal. I have a test in bug 786295 that seems to work as expected. Basically I have this setup:

  <!-- test_webapp_clearBrowserData.html -->
  <iframe mozbrowser
          mozapp="http://example.org/manifest.webapp"
          remote="true"
          src="webapp_clearBrowserData_appFrame.html"/>
    <!-- process boundary -->
    <!-- webapp_clearBrowserData_appFrame.html -->
    <iframe mozbrowser
            src="webapp_clearBrowserData_browserFrame.html"/>
      <!-- webapp_clearBrowserData_browserFrame.html -->
      <!-- stuff that uses IndexedDB -->

In this test I get indexedDB data for the inner-most browser frame with origin "1+t+http+++example.org", which is correct. To verify you can pause the test before the clearing operation and look in the directory <profile>/indexedDB/. So whatever I'm doing in this test shows that our current code works as expected.

However, on B2G, I see brokenness. I don't know of any automated tests that expose this problem, but the STR are simple:

  1. Open The browser application on B2G (I use desktop b2g builds, but I'd assume
     the same thing would happen on the device too).
  2. Enter "robnyman.github.com/html5demos/indexeddb" in the URL bar and go.
  3. Look in the directory: <profile>/indexedDB/.
  4. Observe "19+f+app+++browser.gaiamobile.org" and "0+t+http+++robnyman.github.com".

I guess the browser appId could be different on your machine, not sure how that works. But in any case, the robnyman origin should have the same appId as the browser app, with a "t" instead of "f". I consistently see 0 as the appId for this origin.
Flags: needinfo?(bent.mozilla)
> So here's the deal.

This is perfect (for now, anyway).  Thanks a ton.
Note that this almost certainly also affect where cookies and appcached content goes. The cookie and appcache code uses nsILoadContext (implemented by docshell) to figure out which datajar to store their data in.

Unfortunately I don't have any steps to reproduce which are as simple.

One way to reproduce it would be to remove the hack at [1] and then check that pressing the "clear private data" button in the browser UI actually clears cookies. For example, it should log you out of bugzilla.

[1] http://hg.mozilla.org/mozilla-central/file/5c82f5a5e90d/dom/apps/src/Webapps.jsm#l1742
I have patches which pass bent's testcase.  I need to clean them up, do some more testing, and see if I can write an automated test here.
It's a long name, but at least it's clear.  (We originally considered GetSameTypeParentIgnoreBrowserFrame and rejected it as too long.  But I figure as long as I'm changing this for clarity, we might as well go all the way.)
Attachment #675730 - Flags: review?(bzbarsky)
This patch doesn't cover the entire rename, but it gets most of them.
Attachment #675734 - Flags: review?(bzbarsky)
Again, this doesn't encompass the changes to all users of this interface, but it gets most of them.
Attachment #675736 - Flags: review?(bzbarsky)
This changes the interface of nsIDocShell to be more clear about the
app/browser distinction.  The biggest change is to how you mark a
docshell as being a browser or part of an app -- now you do either

  SetIsApp(appId)

or

  SetIsBrowserInsideApp(appId)

.  To create a browser outside an app (something we don't do in B2G),
you pass NO_APP_ID to SetIsBrowserInsideApp.
Attachment #675738 - Flags: review?(bzbarsky)
This changes nsFrameLoader to use the modified nsIDocShell and TabParent interfaces, and also clarifies the browser/app distinction within the frameloader code.
Attachment #675739 - Flags: review?(bzbarsky)
This actually fixes bug 802366, passing the right parameters to the docshell so that browser frames get the app id of their containing apps.

The order of arguments to many functions is changed from (is-browser, app) to (app, is-browser), for consistency with other modules.

"App" is replaced with "OwnOrContainingApp" where appropriate to emphasize that we may have inherited this app from our parent process.
Attachment #675740 - Flags: review?(jones.chris.g)
Attachment #672035 - Attachment is obsolete: true
This is a lot of mostly un-interesting changes; sorry, Boris and Chris.  Hopefully it won't be a problem to take such a large change on Aurora.  Thankfully most of the non-trivial changes here are to B2G-only code.

Basically, I felt like the main bug here was that in half of our interfaces, we used "is browser frame/element" to mean "browser or app", and in the other half, we used it to mean "is browser not app".  I've fixed that, so now it's clear what's going on.  Once I fixed the interfaces, the functional bug wasn't hard to resolve.

I also modified Tab{Parent,Child} and nsFrameLoader to call "app" "ownOrContainingApp", to emphasize that we might have inherited the app from a parent process.  I left nsIDocShell::appId alone, because changing that would have necessitated changing nsILoadGroup and a /lot/ of users in Necko.  I also don't feel like nsILoadGroup::appId is as confusing as TabParent::appId, because you always want the loadgroup's own or containing appId, but you sometimes care that the tabparent/frameloader's appid is inherited.

I'll look into cooking up an automated test for this, but I wanted to get these patches out in the meantime.
Summary: Make the browser vs. app distinction clear, make our APIs sane → Make the browser vs. app distinction clear, make our APIs sane, and let browser processes inherit their parents' app-ids
Comment on attachment 675740 [details] [diff] [review]
Bug 802366 - Part 3c, the main event: dom/ipc changes.

Much better, thanks!
Attachment #675740 - Flags: review?(jones.chris.g) → review+
Comment on attachment 675728 [details] [diff] [review]
Prelude, part 1: Rename in-process-browser-frame-shown to in-process-browser-or-app-frame-shown.

r=me
Attachment #675728 - Flags: review?(bzbarsky) → review+
Comment on attachment 675730 [details] [diff] [review]
Prelude, part 2: Rename GetParentIgnoreBrowserFrame to GetSameTypeParentIgnoreBrowserAndAppBoundaries.

Please rev the docshell iid.

r=me with that.
Attachment #675730 - Flags: review?(bzbarsky) → review+
Comment on attachment 675731 [details] [diff] [review]
Bug 802366 - Prelude, part 3: Make some methods on nsIPrincipal infallible, and improve documentation on other methods.

Can you not nix more C++ blocks there?

In any case, r=me.  Much better comments!
Attachment #675731 - Flags: review?(bzbarsky) → review+
Comment on attachment 675732 [details] [diff] [review]
Bug 802366 - Prelude, part 4: Use and simplify nsScriptSecurityManager::GetDocShellCodebasePrincipal.

r=me
Attachment #675732 - Flags: review?(bzbarsky) → review+
Comment on attachment 675733 [details] [diff] [review]
Bug 802366 - Prelude, part 5: Improve comments in nsIDocShellTreeItem.idl and nsILoadContext.idl.

s/This is call/This getter/ and r=me.
Attachment #675733 - Flags: review?(bzbarsky) → review+
Comment on attachment 675736 [details] [diff] [review]
Bug 802366 - Part 2 (fold into main part): Changes to nsIMozBrowserFrame (ReallyIsBrowser --> ReallyIsBrowserOrApp) and users of the interface.

r=me
Attachment #675736 - Flags: review?(bzbarsky) → review+
Comment on attachment 675734 [details] [diff] [review]
Bug 802366 - Part 1 (fold into main part): s/GetIsContentBoundary/GetIsBrowserOrApp/ s/GetIsBelowContentBoundary/GetIsInBrowserOrApp/.

r=me
Attachment #675734 - Flags: review?(bzbarsky) → review+
Comment on attachment 675738 [details] [diff] [review]
Bug 802366 - Part 3a, the main event: docshell changes.

r=me
Attachment #675738 - Flags: review?(bzbarsky) → review+
Comment on attachment 675739 [details] [diff] [review]
Bug 802366 - Part 3b, the main event: frameloader changes.

r=me.

Thank you for breaking this up into sane-sized chunks!
Attachment #675739 - Flags: review?(bzbarsky) → review+
> Can you not nix more C++ blocks there?

You know, I didn't even notice those.

I think I can nix them.  Strange that it compiled at all!

> Please rev the docshell iid.

For those who didn't read the patch queue: I do this in a later patch (sorry!).  I'll figure out some order to land the patches in so that I rev the IID the first time I touch the interface.

> Thank you for breaking this up into sane-sized chunks!

I'm happy it was useful!  Thanks a lot for the quick reviews (cjones too!).
Try run for 5acfc98200e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5acfc98200e7
Results (out of 67 total builds):
    success: 51
    warnings: 6
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-5acfc98200e7
bent, with respect to writing a test for this: It seems that the mochitest you added in bug 786295 should work as a test for this bug if we changed it so that the app frame was loaded in-process and the browser frame was loaded oop.  (Right now, it's the opposite.)

But when I make the app frame in-process, I get the following exception:

JavaScript error: http://example.org/tests/dom/indexedDB/test/webapp_clearBrowserData_appFrame.html, line 37: indexedDB is null

This is a clean build of trunk; all I've done is change setAttribute("remote", "true") to setAttribute("remote", "false").  Do you have any idea what might be going on?
Flags: needinfo?(bent.mozilla)
Oh, I see.  I filed bug 806127.
Depends on: 806127
Flags: needinfo?(bent.mozilla)
Blocks: 806168
No longer depends on: 806127
Comment on attachment 675922 [details] [diff] [review]
Epilogue: Remove workaround for bug 802366 in Webapps.jsm.

Stealing this one
Attachment #675922 - Flags: review?(bent.mozilla) → review+
By the way, when landing this, we should send an email to dev-b2g saying that it's expected that this will cause all in-browser cookies to get lost. I.e. you'll automatically get logged out from all your websites and lose any localStorage/indexedDB data that any website has created.

It's just a one-shot loss though, so not something that's expected to affect anyone but dogfooders.
Sounds good.  I can land once I get a review in bug 806168.  (I'd like to land with test coverage.)
a6a847452dbf had the wrong bug number in the commit message, so I backed out and relanded that one.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a9450a82af
https://hg.mozilla.org/integration/mozilla-inbound/rev/d718d87d4bb5
https://hg.mozilla.org/mozilla-central/rev/d718d87d4bb5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Wait, this got backed out...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Unfortunately I can't reproduce the test failure locally.

Chris, do you have any insight as to why I'd get

[Child 2198] ###!!! ABORT: creating CanvasLayer back buffer failed!: file ../../../gfx/layers/basic/BasicCanvasLayer.cpp, line 443

?
That will happen if we OOM or if the IPC channel has been closed.
Well, the next line is 

[Parent 2179] WARNING: pipe error (40): Connection reset by peer: file ../../../ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 421

so I'd guess the child is dying before the parent?  But it's strange that we'd cause OOM here, so maybe something else is going on.
That means the ipc socket was closed unexpectedly, which would tend to suggest that the parent wasn't in the middle of shutting down.  But I agree, OOM would be odd.
(i.e. the unexpected close was most likely caused by the child aborting.)
I took a look at the reftest failure, which seems more tractable (in particular, since I can reproduce it).

What's happening is that layout/reftests/reftest-sanity/invalidation.html is hanging.  It's hanging because MozReftestInvalidate is never fired.

Perhaps a hint to what's going on is the fact that when I run with my patches, I get

  [Child process] WARNING: "Flushing retained layers!" from nsLayoutUtils::PaintFrame, when it's called from CanvasRenderingContext2D::DrawWindow.

I don't get that warning without my patches.

I'm trying to figure out how any of this code should be affected by these changes and not coming up with much.  Any ideas would be appreciated.
Aha!  Okay, the problem (at least for reftest) is in TabChild::IsRootContentDocument().  Here's what it originally did (complete with the confusing variable names):

bool
TabChild::IsRootContentDocument()
{
    // (mIsBrowserElement means browser-or-app, I think.)
    if (mIsBrowserElement || mAppId == nsIScriptSecurityManager::NO_APP_ID) {
        // The child side of a browser or app element always behaves like a root
        // content document.
        return true;
    }

    // Otherwise, we're the child side of an <html:app remote=true>
    // embedded in an outer <html:app>.  These don't behave like root
    // content documents in nested contexts.  Because of bug 761935,
    // <html:browser remote> and <html:app remote> can't nest, so we
    // assume this isn't the root.  When that bug is fixed, we need to
    // revisit that assumption.
    return false;
}

That comment is referring to something which currently can't happen.  I even say it can't happen elsewhere in the patches, in ContentParent::CreateBrowserOrApp:

    if (aIsBrowserElement || !aOwnOrContainingApp) {
      // ...
      return;
    }

    // If we got here, we have an app and we're not a browser element.  In this
    // case, we assume the app is our own app, not a containing one.  That is,
    // if you're a remote iframe inside an app, you must either be a browser or
    // a new app; you can't be a non-browser non-app iframe.

Chris, can I return true unconditionally for TabChild::IsRootContentDocument(), with a similar comment?
Blocks: 808206
> Chris, can I return true unconditionally for TabChild::IsRootContentDocument(), with a 
> similar comment?
>> Seems fine to me.

We established on IRC that this is not in fact fine; we need to return false for <mozapp> inside <mozapp>, because we want the inner app's iframe to be transparent.

I designed these patches around the idea that we'd never treat <mozapp> inside <mozapp> different from a stand-alone mozapp -- indeed, that was a basic assumption underlying the idea that the (appid, isbrowser) tuple was all we needed to attach to a frame.

So fixing this in a sane way requires basically rewriting most of these patches so we always carry the full information -- now (appid, containing-app-id, is-browser).  We could maybe hack around this, but I think this bug demonstrates the harm of this sort of hack.  Also, this bug isn't the last time this code will be touched (see bug 808206).

I think I know basically what needs to be done here; I just need some time.  Jason and I will coordinate to make sure that my new solution works for his purposes.
I need to clean up these patches and finish the automated test, but I think it's working.

https://tbpl.mozilla.org/?tree=Try&rev=c45bbb2827e9
Try run for bbdb857a544b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bbdb857a544b
Results (out of 2 total builds):
    exception: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-bbdb857a544b
Try run for c45bbb2827e9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c45bbb2827e9
Results (out of 4 total builds):
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-c45bbb2827e9
Try run for 02ce04d6ea55 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=02ce04d6ea55
Results (out of 57 total builds):
    success: 47
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-02ce04d6ea55
Try run for 02ce04d6ea55 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=02ce04d6ea55
Results (out of 60 total builds):
    success: 49
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-02ce04d6ea55
Try run for 02ce04d6ea55 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=02ce04d6ea55
Results (out of 61 total builds):
    success: 49
    warnings: 11
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-02ce04d6ea55
Try run for bbdb857a544b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bbdb857a544b
Results (out of 3 total builds):
    exception: 2
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-bbdb857a544b
Try run for c45bbb2827e9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c45bbb2827e9
Results (out of 5 total builds):
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-c45bbb2827e9
This lets us make the correct decision in TabChild::IsRootContentDocument.  It's also nicer in general, I think, because TabChild, TabParent, and TabContext all expose the same interface.
Attachment #679066 - Flags: review?(jones.chris.g)
This is a change in behavior, but I think it's more correct than the old behavior.
Attachment #679067 - Flags: review?(bent.mozilla)
I'd really like to have a test for this app-in-app behavior, but it's hard to write, and we need to fix this bug.  I'll see if I can get to it sometime soon, but I'm not going to lose sleep over it.  I have other bugs to lose sleep over.  :)
Comment on attachment 679067 [details] [diff] [review]
Volume II, Part 2: Use TabContext to make IDB handling more correct in TabParent.

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

Yeah, this looks right. Thanks!
Attachment #679067 - Flags: review?(bent.mozilla) → review+
Comment on attachment 679066 [details] [diff] [review]
Volume II part 1: Add and use TabContext, which holds app-in-app data.

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

>+// This is equivalent to AppFrameIPCTabContext with all fields set to NO_APP_ID.
>+struct VanillaFrameIPCTabContext

Not too enthusiastic about this name, but I don't have a better idea,
so OK!

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>-bool
> TabChild::IsRootContentDocument()
> {

>+    // (It would probably make sense if <html:iframe remote=true> or
>+    // <xul:browser remote=true> inside an app was also not a root content
>+    // document.  But that breaks our tests, and it's not a configuration we
>+    // ship.)
> 

Well, <app> within <app> is supposed to behave like <iframe> within a
content document wrt painting.  The confusing part is that we're using
"is root content document" to compute a graphics trait, which sort of
doesn't make sense here.

>diff --git a/dom/ipc/TabContext.cpp b/dom/ipc/TabContext.cpp

>+TabContext::TabContext()
>+  : mInitiailized(false)

mInitialized

>+      TabContext *context;
>+      if (ipcContext.openerParent()) {
>+        context = static_cast<TabParent*>(ipcContext.openerParent());
>+        if (context->IsBrowserElement() && !ipcContext.isBrowserElement()) {
>+          // If the TabParent corresponds to a browser element, then it can only
>+          // open other browser elements, for security reasons.  We should have
>+          // checked this before calling the TabContext constructor, so this is
>+          // a fatal error.
>+          MOZ_CRASH();

MOZ_NOT_REACHED() has the same effect and may be clearer, but I don't
particularly care which you use.

>+bool
>+TabContext::SetTabContext(const TabContext& aContext)
>+{
>+  NS_ENSURE_FALSE(mInitiailized, false);
>+

Any reason why we shouldn't assert !mInitialized?

dom/ipc doesn't do control-flow affecting macros.  Explicit return
stmts please.

>+IPCTabContext
>+TabContext::AsIPCTabContext() const
>+{
>+  if (mIsBrowser) {
>+    BrowserFrameIPCTabContext context;
>+    context.browserFrameOwnerAppId() = mContainingAppId;
>+    return IPCTabContext(context);

|return BrowserFrameIPCTabContext(mContainingAppId)| is equivalent and
 much cleaner.

>+  AppFrameIPCTabContext context;
>+  context.ownAppId() = mOwnAppId;
>+  context.appFrameOwnerAppId() = mContainingAppId;
>+  return IPCTabContext(context);

Similarly, |return AppFrameIPCTabContext(mOwnAppId, mContainingAppId)|.

>diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h

>+  bool mInitiailized;
>+  bool mIsBrowser;

Reorder these after the uint32_t members.

OK, this looks really really good.  r=me with comments above addressed.
Attachment #679066 - Flags: review?(jones.chris.g) → review+
> Well, <app> within <app> is supposed to behave like <iframe> within a
> content document wrt painting.  The confusing part is that we're using
> "is root content document" to compute a graphics trait, which sort of
> doesn't make sense here.

Thanks; I updated the comment to try to make it clearer.

>diff --git a/dom/ipc/TabContext.cpp b/dom/ipc/TabContext.cpp

> MOZ_NOT_REACHED() has the same effect and may be clearer, but I don't
> particularly care which you use.

I'd considered that, but I rejected it based on comments in MFBT.
MOZ_NOT_REACHED() expands to |MOZ_ASSERT(false); MOZ_NOT_REACHED_MARKER();|,
and the comment on MOZ_NOT_REACHED_MARKER() says:

  "[I]t is undefined behavior for execution to reach this point.  No guarantees
  are made about what will happen if this is reached at runtime."

I guess I see the purpose of MOZ_NOT_REACHED() existing as described in the
comments (particularly since it indicates noreturn), but ISTM that an assertion
of the sort you describe ought to exist as well.

>+bool
>+TabContext::SetTabContext(const TabContext& aContext)
>+{
>+  NS_ENSURE_FALSE(mInitiailized, false);
>+

> dom/ipc doesn't do control-flow affecting macros.  Explicit return
> stmts please.

Heh, I think you may have lost this battle some time ago:

$ git grep NS_ENSURE dom/ipc | cut -d ':' -f 1 | uniq -c | sort -rn
  31 dom/ipc/Blob.cpp
  17 dom/ipc/ContentParent.cpp
  15 dom/ipc/ContentChild.cpp
  13 dom/ipc/TabChild.cpp
  11 dom/ipc/TabParent.cpp
   9 dom/ipc/TabContext.cpp
   5 dom/ipc/ProcessPriorityManager.cpp
   2 dom/ipc/TabMessageUtils.cpp
   2 dom/ipc/PermissionMessageUtils.cpp

Is it OK if I leave these in?

>diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h

>>+  bool mInitiailized;
>>+  bool mIsBrowser;
>
> Reorder these after the uint32_t members.

I moved mInitialized to the beginning, because I think that makes more sense
than having it at the end of the members.
(In reply to Justin Lebar [:jlebar] from comment #74)
> >+bool
> >+TabContext::SetTabContext(const TabContext& aContext)
> >+{
> >+  NS_ENSURE_FALSE(mInitiailized, false);
> >+
> 
> > dom/ipc doesn't do control-flow affecting macros.  Explicit return
> > stmts please.
> 
> Heh, I think you may have lost this battle some time ago:
> 
> Is it OK if I leave these in?
> 

Grumble.  10 review demerits ;).
I needed to make the IDB handling do the same sort of "allow non-browsers to
read browsers, but not vice versa" thing that we do for popups.  Hopefully this
is the last patch here; this bug is becoming unweildy!
Attachment #680223 - Flags: review?(bent.mozilla)
Attachment #680223 - Flags: review?(bent.mozilla) → review+
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out for Windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc1c0530b00

https://tbpl.mozilla.org/php/getParsedLog.php?id=16912797&tree=Mozilla-Inbound

e:/builds/moz2_slave/m-in-w32/build/dom/indexedDB/IndexedDatabaseManager.cpp(621) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int

e:/builds/moz2_slave/m-in-w32/build/dom/indexedDB/IndexedDatabaseManager.cpp(621) : error C2143: syntax error : missing ',' before '&'

e:/builds/moz2_slave/m-in-w32/build/dom/indexedDB/IndexedDatabaseManager.cpp(623) : error C2511: 'bool mozilla::dom::indexedDB::IndexedDatabaseManager::TabContextMayAccessOrigin(const int)' : overloaded member function not found in 'mozilla::dom::indexedDB::IndexedDatabaseManager'

        e:\builds\moz2_slave\m-in-w32\build\dom\indexeddb\IndexedDatabaseManager.h(47) : see declaration of 'mozilla::dom::indexedDB::IndexedDatabaseManager'
This was causing Crashtest-IPC timeouts too.

https://tbpl.mozilla.org/php/getParsedLog.php?id=16913805&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/dom/indexedDB/crashtests/726376-1.html | application timed out after 330 seconds with no output
PROCESS-CRASH | file:///home/cltbld/talos-slave/test/build/reftest/tests/dom/indexedDB/crashtests/726376-1.html | application crashed (minidump found)
> I believe these were posted to the wrong bug.

It's basically become one giant patch queue at this point, since patches from this bug have to land before and after patches from other bugs, in order to have any hope of all csets compiling.  Which, to be fair, I haven't done a good job of by any measure.

Sorry.  :(
We are really too clever for our own good with these ascii origins.  In particular, the ascii origin for (no-browser, no-app [i.e., app 0], mozilla.org) is not the same as "mozilla.org".  :-/

Anywho, this is a simple fix, I think.

https://tbpl.mozilla.org/?tree=Try&rev=f6e00ea3e664
> It's basically become one giant patch queue at this point, since patches from this bug 
> have to land before and after patches from other bugs

Actually, that's not true at all.  This bug regains a modicum of sanity.

And a -Wunused -Werror fix for macos.

https://tbpl.mozilla.org/?tree=Try&rev=07ee76cc1f02
And again with working trychooser params.  (Sorry for the spam.)

https://tbpl.mozilla.org/?tree=Try&rev=61ff8164a3f9
Depends on: 810610
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: