1303943 - Don't inherit principal for external loads and update documentation for principalToInherit

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 1297338 we introduced the concept of a PrincipalToInherit, but we still need to update documentation.

In particular (within nsIDocShell) we should document when either principalToInherit or TriggeringPrincipal are allowed to be null and how they interact.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that my review in bug 1297338 was conditional on me having read the documentation and verified that my understanding of what's going on matches what's actually going on...

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Boris, hopefully I can answer all of your questions from:
  https://bugzilla.mozilla.org/show_bug.cgi?id=1297338#c62

I did an audit of all the callsites to InternalLoad, there are 7 total (see underneath) and I tried to summarize the relation between triggeringPrincipal and principalToInherit with some Matrix like example within (5). Hopefully that clears things up:

(1) ::LoadErrorPage()
    triggeringPrincipal = nullptr;
    principalToInherit  = nullptr;
    referrer = nullptr;
    No referrer and no triggeringPrincipal means we create a SystemPrincipal and use that as
    TriggeringPrincipal. No principalToInherit means we fall back to the triggeringPrincipal
    within the loadInfo.

(2) ::Reload()
    (a) triggeringPrincipal = nullptr;
        principalToInherit  = nullptr;
        referrer = mReferrerURI;
        Creates principal from the referrer if not null and uses that a triggeringPrincipal,
        else falls back to the systemPrincipal for both.

    (b) triggeringPrincipal = doc->NodePrincipal()
        principalToInherit = doc->NodePrincipal()
        referrer = mReferrerURI;
        Uses the NodePrincipal() as the triggeringPrincipal as well as the principalToInherit
        (doc->NodePrincipal() should never be null).

(3) ::LoadHistoryEntry
    triggeringPrincipal = queried from nsISHEntry
    principalToInherit  = queried from nsISHEntry
    referrer = queried from nsISHEntry
    Uses the the same triggering and principalToInherit as stored in nsISHEntry.
   
(4) OnLinkClickSync
    triggeringPrincipal = aContent->NodePrincipal()
    principalToInherit  = aContent->NodePrincipal()
    referrer = refererDoc->GetDocumentURI();
    Uses the NodePrincipal() as the triggeringPrincipal as well as the principalToInherit
    (aContent->NodePrincipal() should never be null).

(5) LoadURI()
    This one is the more tricky one and needs some more explanation.
    That method queries the triggeringPrincipal from the passed in docShelloadinfo.
    That triggeringPrincipal is then used as the principalToInherit.
    While the triggeringPrincipal itself remains untouched, the principalToInherit might be updated to:

    triggeringPrincipal       principalToInherit
    -------------------       ------------------
    nullptr               =>  nullptr
                              NullPrincipal   [if LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL]

    SystemPrincipal       =>  SystemPrincipal [if typeChrome load]
                              nullptr         [if !principalIsExplicit]
                              NullPrincipal   [if LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL]

    CodeBasePrincipal     =>  CodeBasePrincipal [default]
                              NullPrincipal   [if LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL]



    Within InternalLoad() the triggeringPrincipal might be updated before creating the
    loadInfo for the channel:

    aTriggeringPrincipal      triggeringPrincipal
    --------------------      -------------------
    CodeBasePrincipal     =>  CodeBasePrincipal
    SystemPrincipal       =>  SystemPrincipal
    nullptr               =>  CodeBasePrincipal (if aReferrer != null)
                              SystemPrincipal   (if aReferrer == null)



(6) Recursive call from within InternalLoad()
(7) InternalLoadEvent()
    uses icoming args from internalLoad, so it might be any of the 5 cases above.



Looking at all that, I am proposing the following documentation for the arguments of internalLoad() within nsIDocShell.idl:

> @param aTriggeringPrincipal
Principal that initiated that load. If passing null for this argument, then internally a principal is created from aReferrer. If aReferrer is also null, then the triggeringPrincipal defaults to the SystemPrincipal. Please note that this is the principal that is used for security checks. If the argument aURI is provided by the web, then please pass an explicit triggeringPrincipal to avoid the fallback to SystemPrincipal and hence a potential security risk.

> @param aPrincipalToInherit
Principal to be inherited for that load. If passing null for this argument, then internally the triggeringPrincipal is also used for the principalToInherit. There are cases where those two principals need to be different though. E.g. the system might initiate a load for 'about:blank', hence SystemPrincipal is passed for aTriggeringPrincipal. But the principal to be inherited for that load should be a NullPrincipal and not the SystemPrincipal. In that case, please pass a non null principalToInherit.


What do you think? Are you seeking for more information?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Boris, comment 2 is my first attempt to bring some light into the dark. Please let me know if I can provide more debug information so we can update the documentation for principalToInherit. Thanks!

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here.  This is at the top of my priority list for Monday morning....

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, I ended up with more time than I thought today, and got a chance to read through this stuff.

Thank you for putting comment 2 together.  It's very helpful.

Your proposed documentation looks good, but I would suggest one more addition to it.  It looks like in most cases both principals are null or both are non-null.  The exceptions are:

1) LoadURI with null triggering principal and LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL.
2) LoadURI with system triggering principal and non-chrome load.  But in this case INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is always set.

Could we please assert (and document) that if aPrincipalToInherit is null then either aTriggeringPrincipal is null or INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is set?

And could we assert (and document) that if aTriggeringPrincipal is null then either aPrincipalToInherit is null or it's a NullPrincipal?

Now that we have written this out, it also worries me that we have this situation where we can end up using a system principal as the principal to inherit, just because null got passed in principalToInherit.  Specifically, this can happen in the following ways:

* aTriggeringPrincipal is system, aPrincipalToInherit is null (so INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is set), and aLoadType == LOAD_NORMAL_EXTERNAL, so we never end up actually inheriting from the current document.  

* aTriggeringPrincipal is null, aPrincipalToInherit is null, aReferer is null or we fail to create a principal from it.

In both of these cases right now we will end up with a system triggeringPrincipal per your table, and a null principalToInherit, and so will inherit the system principal.  I think it would be safer to set principalToInherit to a NullPrincipal in both of these cases.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1297338_add_principaltoinherit_documentation.patch

Boris, I updated the documentation and incorporated your suggestions. When running a few tests locally however, I encountered some errors:

1) docshell/test/navigation/browser_bug343515.js
  -> aTriggeringPrincipal: SystemPrincipal
  -> aPrincipalToInherit: nullptr

  The load initiates from LoadHistoryEntry() which does not set
  the INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL flag, see stacktrace [1].
  Probably also worth pointing out is this comment within LoadHistoryEntry:
  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#12465


2) docshell/test/chrome/test_bug789773.xul
   -> aTriggeringPrincipal: SystemPrincipal
   -> aPrincipalToInherit: nullptr

   In which case we used to fall back to the SystemPrincipal as the
   principalToInherit, but now causes the test to fail, because
   principalToInherit is set to a NullPrincipal, see stacktrace [2].


[1] --- stacktrace for browser_bug343515.js ---
Assertion failure: aPrincipalToInherit || (!aTriggeringPrincipal || aTriggeringPrincipal->GetIsNullPrincipal() || (aFlags & INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL)), at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:9796
#01: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:9793 (discriminator 1))
#02: nsDocShell::LoadHistoryEntry(nsISHEntry*, unsigned int) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:12580)
#03: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:1437)
#04: nsSHistory::InitiateLoad(nsISHEntry*, nsIDocShell*, long) (/home/ckerschb/moz/mc/docshell/shistory/nsSHistory.cpp:1782)
#05: nsSHistory::CompareFrames(nsISHEntry*, nsISHEntry*, nsIDocShell*, long, bool*) (/home/ckerschb/moz/mc/docshell/shistory/nsSHistory.cpp:1672)
#06: nsSHistory::LoadEntry(int, long, unsigned int) (/home/ckerschb/moz/mc/docshell/shistory/nsSHistory.cpp:1607)
#07: nsSHistory::GoBack() (/home/ckerschb/moz/mc/docshell/shistory/nsSHistory.cpp:851)


[2] --- stacktrace for test_bug789773.xul ---
#01: nsGlobalWindow::SetInitialPrincipalToSubject() (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:2194 (discriminator 1))
#02: nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, float*, mozIDOMWindowProxy**) (/home/ckerschb/moz/mc/embedding/components/windowwatcher/nsWindowWatcher.cpp:1141 (discriminator 1))
#03: nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, float, unsigned char, mozIDOMWindowProxy**) (/home/ckerschb/moz/mc/embedding/components/windowwatcher/nsWindowWatcher.cpp:461 (discriminator 3))
#04: nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsPIDOMWindowOuter**) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:11900)
#05: nsGlobalWindow::OpenJS(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsPIDOMWindowOuter**) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:8144)
#06: nsGlobalWindow::OpenOuter(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:8102)
#07: nsGlobalWindow::Open(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (/home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:8111 (discriminator 1))
#08: open (/home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/WindowBinding.cpp:2202)
#09: genericMethod (/home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/WindowBinding.cpp:14755)
#10: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/ckerschb/moz/mc/js/src/jscntxtinlines.h:239)
#11: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:458)
#12: InternalCall (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:504)
#13: js::CallFromStack(JSContext*, JS::CallArgs const&) (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:510)
#14: Interpret (/home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:2922)

Comment 7

[Boris Zbarsky [:bzbarsky]]

>  Probably also worth pointing out is this comment within LoadHistoryEntry:

I assume you meant the "Do not inherit principal from document (security-critical!)" comment?  Looks like lines have shifted; should use permalinks....

Why did we end up with a null principalToInherit in the history entry?  

> but now causes the test to fail, because principalToInherit is set to a NullPrincipal, see stacktrace

Is this ending up in the "aTriggeringPrincipal is null, aPrincipalToInherit is null, aReferer is null or we fail to create a principal from it." case?  That is, what is the callstack to the call with null aPrincipalToInherit here?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> >  Probably also worth pointing out is this comment within LoadHistoryEntry:
> 
> I assume you meant the "Do not inherit principal from document
> (security-critical!)" comment?  Looks like lines have shifted; should use
> permalinks....
> 
> Why did we end up with a null principalToInherit in the history entry?  

The problem is buried here in nsDocShell::AddToSessionHistory() where we potentially end up in a situation where we set the triggeringPrincipal but not the principalToInherit. So we can either fix that within AddToSessionHistory right before we call  entry->Create() and set the principalToInherit to triggeringPrincipal if null, or we can also do a fallback within nsSHEntry::Create(). I think it makes sense to add that fallback to nsSHEntry::Create(), or alternatively we could also do that within nsSHEntry::GetPrincipalToInherit() and return the triggeringPrincipal if mPrincipalToInherit is null.

[1] --- stacktrace for browser_bug343515.js ---
Assertion failure: false (uahaha), at /home/ckerschb/moz/mc/docshell/shistory/nsSHEntry.cpp:466
#01: nsSHEntry::Create(nsIURI*, nsAString_internal const&, nsIInputStream*, nsILayoutHistoryState*, nsISupports*, nsACString_internal const&, nsIPrincipal*, nsIPrincipal*, unsigned long, bool) (/home/ckerschb/moz/mc/docshell/shistory/nsSHEntry.cpp:466)
#02: nsDocShell::AddToSessionHistory(nsIURI*, nsIChannel*, nsIPrincipal*, nsIPrincipal*, bool, nsISHEntry**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:12393)
#03: nsDocShell::OnNewURI(nsIURI*, nsIChannel*, nsIPrincipal*, nsIPrincipal*, unsigned int, bool, bool, bool) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:11779)
#04: nsDocShell::OnLoadingSite(nsIChannel*, bool, bool) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:11849)
#05: nsDocShell::CreateContentViewer(nsACString_internal const&, nsIRequest*, nsIStreamListener**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:9174)
#06: nsDSURIContentListener::DoContent(nsACString_internal const&, bool, nsIRequest*, nsIStreamListener**, bool*) (/home/ckerschb/moz/mc/docshell/base/nsDSURIContentListener.cpp:128)
#07: nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (/home/ckerschb/moz/mc/uriloader/base/nsURILoader.cpp:722)
#08: nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (/home/ckerschb/moz/mc/uriloader/base/nsURILoader.cpp:396 (discriminator 1))
#09: nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) (/home/ckerschb/moz/mc/uriloader/base/nsURILoader.cpp:259)
#10: mozilla::net::nsHttpChannel::CallOnStartRequest() (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:1308)
#11: mozilla::net::nsHttpChannel::ContinueProcessNormal(nsresult) (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:2359)
#12: mozilla::net::nsHttpChannel::ProcessNormal() (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:2295)
#13: mozilla::net::nsHttpChannel::ContinueProcessResponse1(nsresult) (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:2083)
#14: mozilla::net::nsHttpChannel::ProcessResponse() (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:2046)
#15: mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:6436)

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> > but now causes the test to fail, because principalToInherit is set to a NullPrincipal, see stacktrace
> 
> Is this ending up in the "aTriggeringPrincipal is null, aPrincipalToInherit
> is null, aReferer is null or we fail to create a principal from it." case? 
> That is, what is the callstack to the call with null aPrincipalToInherit
> here?

For test_bug789773.xul we end up setting |principaltoInherit = NullPrincipal();| in the case where there aTriggeringPrincipal = null and also aReferrer = null. Please note that the code looks like this:

>    if (aReferrerURI) {
>     rv = CreatePrincipalFromReferrer(aReferrerURI,
>                                       getter_AddRefs(triggeringPrincipal));
>      NS_ENSURE_SUCCESS(rv, rv);
>    } else {
>      triggeringPrincipal = nsContentUtils::GetSystemPrincipal();

In other words, if we fail to create a triggeringPrincipal from the referrer then we bail out instead of falling back to the systemPrincipal. Just making sure we are on the same page because you wrote 'aReferer is null or we fail to create a principal from it'.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Boris, whenever you get a chance, could you have a look at comment 8 and 9? Happy to provide any debug info that helps us move forward with this bug. Thanks for your help.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> The problem is buried here in nsDocShell::AddToSessionHistory() where we potentially
> end up in a situation where we set the triggeringPrincipal but not the principalToInherit

Hmm.  So ideally the session history would store whatever is valid to pass to InternalLoad, since that's where it will get passed.

I realize that by the time we land there we're basically examining whatever got set on the channel, which is not what got passed to InternalLoad, unfortunately.  I guess this could be the the situation where we called InternalLoad with nullptr for both principals, synthesized a triggeringPrincipal from a refererr (or used system if no referrer), but never set up a principaltoinherit, or something.

Concretely, is there a reason this block in AddToSessionHistory:

        } else if (loadInfo->GetForceInheritPrincipal()) {
          principalToInherit = loadInfo->PrincipalToInherit();
        }

is conditioned on loadInfo->GetForceInheritPrincipal() instead of just being unconditional?

> Just making sure we are on the same page 

We are; I just assumed things about the failure behavior without double-checking the code.

OK. So we have a load with no referrer, no triggering principal, no principal to inherit.  This uses a system triggeringPrincipal in the end, and inherits it.  That seems super-sketchy, especially because if triggering principal _started_ as system we would have changed principalToInherit to a nullprincipal in the "not chrome docshell" case in LoadURI, right?

So with that in mind, I think we should do the following:

1)  In the "null principals, null referrer" case, when we set triggeringPrincipal to system, set principalToInherit to nullprincipal if this a non-chrome docshell.  Or will that still fail the docshell/test/chrome/test_bug789773.xul case?  I'm really not sure why we ever end up with a null triggeringPrincipal in that testcase, honestly; which load is that, exactly?  I would have expected us to always have a triggeringPrincipal there...

2)  If that works, then in a followup bug I think we can check whether the cases in Reload() that end up doing the "triggeringPrincipal = nullptr, principalToInherit  = nullptr, referrer = mReferrerURI" ever get hit in practice and if not whether we can just remove them.  If we can, then looks to me like we should be able to change LoadErrorPage to use a system triggering principal explicitly, hoist the logic that creates a triggering principal from referrer up to LoadURI (since it will be the only thing that can pass an interesting referrer but a null triggering principal), do the "create from referer" thing before our current computation of principalToInherit, and if there is no referrer, explicitly set triggeringPrincipal to system _before_ doing the principalToInherit computation.

Basically, try to trim down this forest of different paths and cases to be smaller and saner... because what we have now is completely impossible to reason about, which is bad for security code.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1297338_add_principaltoinherit_documentation.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> Concretely, is there a reason this block in AddToSessionHistory:
> 
>         } else if (loadInfo->GetForceInheritPrincipal()) {
>           principalToInherit = loadInfo->PrincipalToInherit();
>         }
> 
> is conditioned on loadInfo->GetForceInheritPrincipal() instead of just being
> unconditional?

No, there is no particular reason why we set the principalToInherit only conditionally. I think my idention was to keep the logic as close to the original logic before we introduced a principalToInherit. In the old world it made sense to have the conditioin because we didn't want to overwrite the already defined triggeringPrincipal. Hence I kept that condition when we introduced the principalToInherit [1]. I think I agree that we can remove the condition.

That change fixes the problems for browser_bug343515.js.

[1] https://hg.mozilla.org/mozilla-central/rev/dee3d68b0fc8#l2.655
 
> I'm really not sure why we
> ever end up with a null triggeringPrincipal in that testcase, honestly;
> which load is that, exactly?  I would have expected us to always have a
> triggeringPrincipal there...

Boris, you are absolutelty right, what I wrote in the first part of comment 9 is *wrong*. In fact we end up in the case where we have a non null aTriggeringPricnipal and that aTriggeringPrincipal is a SystemPrincipal(), hence we set the principalToInherit to a freshly created NullPrincipal (see attached patch). More precisely, when running test_bug789773.xul we get a load for "about:blank" (aContentPolicyType is TYPE_DOCUMENT) (see stacktrace underneath) which enters that branch:

+  if (triggeringPrincipal) {
+    if (!principalToInherit &&
+        nsContentUtils::IsSystemPrincipal(triggeringPrincipal)) {
+      principalToInherit = nsNullPrincipal::CreateWithInheritedAttributes(this);
+    }
+  }

which in turn causes the assertion within nsGlobalWindow.cpp:2194 to get triggered (see second part of comment 6). 

So, I suppose what we want is some special case handling for about:blank loads, so we don't set principalToInherit to a nullPrincipal in that case, but fall back to the systemPrincipal, right?


#01: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString_internal const&, nsIURI*, unsigned int) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10966)
#02: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10738)
#03: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:1587)
#04: nsFrameLoader::ReallyStartLoadingInternal() (/home/ckerschb/moz/mc/dom/base/nsFrameLoader.cpp:492)
#05: nsFrameLoader::ReallyStartLoading() (/home/ckerschb/moz/mc/dom/base/nsFrameLoader.cpp:376)
#06: nsDocument::MaybeInitializeFinalizeFrameLoaders() (/home/ckerschb/moz/mc/dom/base/nsDocument.cpp:6904 (discriminator 1))
#07: mozilla::dom::XULDocument::DoneWalking() (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:3033)
#08: mozilla::dom::XULDocument::ResumeWalk() (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:2964)
#09: mozilla::dom::XULDocument::OnPrototypeLoadDone(bool) (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:572)
#10: mozilla::dom::XULDocument::EndLoad() (/home/ckerschb/moz/mc/dom/xul/XULDocument.cpp:548)
#11: XULContentSinkImpl::DidBuildModel(bool) (/home/ckerschb/moz/mc/dom/xul/nsXULContentSink.cpp:229)
#12: nsParser::DidBuildModel(nsresult) (/home/ckerschb/moz/mc/parser/htmlparser/nsParser.cpp:901)
#13: nsParser::ResumeParse(bool, bool, bool) (/home/ckerschb/moz/mc/parser/htmlparser/nsParser.cpp:1508)
#14: nsParser::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/ckerschb/moz/mc/parser/htmlparser/nsParser.cpp:1880 (discriminator 1))
#15: nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/ckerschb/moz/mc/netwerk/base/nsBaseChannel.cpp:827)
#16: nsInputStreamPump::OnStateStop() (/home/ckerschb/moz/mc/netwerk/base/nsInputStreamPump.cpp:715)
#17: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/ckerschb/moz/mc/netwerk/base/nsInputStreamPump.cpp:433)
#18: nsInputStreamReadyEvent::Run() (/home/ckerschb/moz/mc/xpcom/io/nsStreamUtils.cpp:97 (discriminator 1))
#19: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:1082 (discriminator 1))
#20: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:290)
#21: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:96)
#22: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:233)
#23: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:226)
#24: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:205)
#25: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:158)
#26: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:283)
#27: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4450)
#28: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4583)
#29: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4674)
#30: do_main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:282)
#31: main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:415)
#32: __libc_start_main (/build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:321)
#33: _start (/home/ckerschb/moz/mc-obj-ff-dbg/dist/bin/firefox)
#34: ??? (???:???)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> That change fixes the problems for browser_bug343515.js.

Excellent.  ;)

> In fact we end up in the case where we have a non null aTriggeringPricnipal
> and that aTriggeringPrincipal is a SystemPrincipal()

OK.  But aPrincipalToInherit is nullptr?

Going back to comment 2, this can happen when coming in via LoadURI and !principalIsExplicit, right?  In this case INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL will be set, as noted in comment 5.  In that comment I was only worried about the aLoadType == LOAD_NORMAL_EXTERNAL, but I guess the other way we can still end up with a null principalToInherit is if there is no current document to inherit from (so the INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL flag has no effect).

Assuming the above is all true, I don't think we should use a nullprincipal in this situation for now, though maybe we will start doing so in some cases once bug 1308889 is fixed. 

Reading carefully through DOURILoad, it's actually even worse: if a non-null aPrincipalToInherit is passed for an about:blank load that principal _will_ be inherited, but if a null one is passed the aTriggeringPrincipal will NOT be inherited into about:blank.  That makes it really annoying to reason about this stuff.  :(

So I think what we should do is this: in InternalLoad, right around this block:

    if (aLoadType != LOAD_NORMAL_EXTERNAL && !principalToInherit &&
        (aFlags & INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL) &&
        NS_SUCCEEDED(nsContentUtils::URIInheritsSecurityContext(aURI,
                                                                &inherits)) &&
        inherits) {
      principalToInherit = GetInheritedPrincipal(true);
    }

change the logic to something like this:

    if (!principalToInherit && 
        NS_SUCCEEDED(nsContentUtils::URIInheritsSecurityContext(aURI,
                                                                &inherits)) &&
        inherits) {
      if (aLoadType != LOAD_NORMAL_EXTERNAL && 
          (aFlags & INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL)) {
        principalToInherit = GetInheritedPrincipal(true);
      }

      if (!principalToInherit && 
          (whatever conditions would lead to a system triggering principal)) {
        // We're going to default to inheriting our system triggering principal, 
        // more or less by accident.  This doesn't seem like a good idea.
        principalToInherit = createNullPrincipalHere();
      }
    }

It's a bit unfortunate that this lives so far away from the actual "hey, the triggering principal is really system" determination in DoURILoad.  So I'd probably be OK with leaving this be until after bug 1308889 is fixed, when there should be less action at a distance going on.  Certainly the problem we're trying to fix here is not a new one the patches here are introducing, nor do I think is it a new one that the principalToInherit changes introduced...

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1297338_add_principaltoinherit_documentation.patch

[r? bz :-)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> > In fact we end up in the case where we have a non null aTriggeringPricnipal
> > and that aTriggeringPrincipal is a SystemPrincipal()
> 
> OK.  But aPrincipalToInherit is nullptr?

Yes, the aPrincipalToInherit is a nullptr.
 
> So I think what we should do is this: in InternalLoad, right around this
> block:

Ok, but we should still keep the fallback to NullPrincipal in case we fall back to the systemPrincipal (triggeringPrincipal == null && aReferrer == null), right?

That all seems to work. I suppose this is what we want in this bug and then follow up within Bug 1308889, agreed?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Ok, but we should still keep the fallback to NullPrincipal in case we fall back to the systemPrincipal
> (triggeringPrincipal == null && aReferrer == null), right?

I was thinking we would hoist that out into InternalLoad too, actually.  Hence being OK with all this happening after bug 1308889...

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1297338_add_principaltoinherit_documentation.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15)
> I was thinking we would hoist that out into InternalLoad too, actually. 
> Hence being OK with all this happening after bug 1308889...

Ok, I think I got it all covered (hopefully). Thanks for your help with this bug! Any chance I could convince you to review yourself?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb1b9f43cc1d30bece9a2bcd9b780603365c80b3

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8800628 [details] [diff] [review]
bug_1297338_add_principaltoinherit_documentation.patch

r=me assuming this passes try

Comment 18

[Carsten Book [:Tomcat]]

does not apply cleanly:

Hunk #3 FAILED at 12294
3 out of 3 hunks FAILED -- saving rejects to file docshell/base/nsDocShell.cpp.rej
patching file docshell/base/nsIDocShell.idl
Hunk #1 FAILED at 126
1 out of 1 hunks FAILED -- saving rejects to file docshell/base/nsIDocShell.idl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_1297338_add_principaltoinherit_documentation.patch

Comment 19

[Carsten Book [:Tomcat]]

never mind, landed but with wrong bug number - will fix this

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f70194e136
Don't inherit principal for external loads and update documentation for principalToInherit. r=bz

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Carsten Book [:Tomcat] from comment #19)
> never mind, landed but with wrong bug number - will fix this

Sorry about that; and thanks Carsten; that's what happens when you fork a bug and also split the patch :-(

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for crashing test with crash stacks involving sessionhistory and docshell:

https://hg.mozilla.org/integration/mozilla-inbound/rev/99d514852ac5228c05301a9f1b9ad01c742b8bae

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=07f70194e136a68f8cc97c8910b0c08dd873eee6
Failure log (example): https://treeherder.mozilla.org/logviewer.html#?job_id=37642996&repo=mozilla-inbound

02:24:15  WARNING -  TEST-UNEXPECTED-FAIL | automation.py | application terminated with exit code 1
02:24:15     INFO -  runtests.py | Application ran for: 0:00:07.682000
02:24:15     INFO -  zombiecheck | Reading PID log: c:\docume~1\cltbld~1.t-x\locals~1\temp\tmpappofupidlog
02:24:15     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/MvwsBHYqTLit6dW0w5rviw/artifacts/public/build/firefox-52.0a1.en-US.win32.crashreporter-symbols.zip
02:24:21     INFO -  mozcrash Copy/paste: C:\slave\test\build\win32-minidump_stackwalk.exe c:\docume~1\cltbld~1.t-x\locals~1\temp\tmpa9z2as.mozrunner\minidumps\e22209a8-bb57-4b06-9cbb-7d3f80dff30d.dmp c:\docume~1\cltbld~1.t-x\locals~1\temp\tmptxaf_f
02:24:31     INFO -  mozcrash Saved minidump as C:\slave\test\build\blobber_upload_dir\e22209a8-bb57-4b06-9cbb-7d3f80dff30d.dmp
02:24:31     INFO -  mozcrash Saved app info as C:\slave\test\build\blobber_upload_dir\e22209a8-bb57-4b06-9cbb-7d3f80dff30d.extra
02:24:31  WARNING -  PROCESS-CRASH | automation.py | application crashed [@ 0x19aebb7d]
02:24:31     INFO -  Crash dump filename: c:\docume~1\cltbld~1.t-x\locals~1\temp\tmpa9z2as.mozrunner\minidumps\e22209a8-bb57-4b06-9cbb-7d3f80dff30d.dmp
02:24:31     INFO -  Operating system: Windows NT
02:24:31     INFO -                    5.1.2600 Service Pack 3
02:24:31     INFO -  CPU: x86
02:24:31     INFO -       GenuineIntel family 6 model 30 stepping 5
02:24:31     INFO -       8 CPUs
02:24:31     INFO -  GPU: UNKNOWN
02:24:31     INFO -  Crash reason:  EXCEPTION_ACCESS_VIOLATION_EXEC
02:24:31     INFO -  Crash address: 0x19aebb7d
02:24:31     INFO -  Process uptime: 7 seconds
02:24:31     INFO -  Thread 0 (crashed)
02:24:31     INFO -   0  0x19aebb7d
02:24:31     INFO -      eip = 0x19aebb7d   esp = 0x0012ec00   ebp = 0x0012ec1c   ebx = 0x7c97d5e0
02:24:31     INFO -      esi = 0x0ef16ce0   edi = 0x7c97b3c0   eax = 0x19aebb7d   ecx = 0x0012ec14
02:24:31     INFO -      edx = 0x7c90e4f4   efl = 0x00010206
02:24:31     INFO -      Found by: given as instruction pointer in context
02:24:31     INFO -   1  ntdll.dll!RtlDispatchException + 0x19
02:24:31     INFO -      eip = 0x7c92a969   esp = 0x0012ec24   ebp = 0x0012ec94
02:24:31     INFO -      Found by: previous frame's frame pointer
02:24:31     INFO -   2  ntdll.dll!KiUserExceptionDispatcher + 0xe
02:24:31     INFO -      eip = 0x7c90e46a   esp = 0x0012ec9c   ebp = 0x0012ef9c
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -   3  xul.dll!nsCOMArray_base::ReplaceObjectAt(nsISupports *,int) [nsCOMArray.cpp:07f70194e136 : 209 + 0x4]
02:24:31     INFO -      eip = 0x03b070a5   esp = 0x0012ecbc   ebp = 0x0012ef9c
02:24:31     INFO -      Found by: call frame info with scanning
02:24:31     INFO -   4  xul.dll!nsSHEntry::AddChild(nsISHEntry *,int) [nsSHEntry.cpp:07f70194e136 : 735 + 0x7]
02:24:31     INFO -      eip = 0x049fb501   esp = 0x0012efa4   ebp = 0x0012efd4
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -   5  xul.dll!nsDocShell::AddChildSHEntry(nsISHEntry *,nsISHEntry *,int,unsigned int,bool) [nsDocShell.cpp:07f70194e136 : 4333 + 0xc]
02:24:31     INFO -      eip = 0x049f0483   esp = 0x0012efdc   ebp = 0x0012eff4
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -   6  xul.dll!nsDocShell::AddChildSHEntryToParent(nsISHEntry *,int,bool) [nsDocShell.cpp:07f70194e136 : 4418 + 0x1e]
02:24:31     INFO -      eip = 0x049f06a1   esp = 0x0012effc   ebp = 0x0012f02c
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -   7  xul.dll!nsDocShell::AddToSessionHistory(nsIURI *,nsIChannel *,nsIPrincipal *,nsIPrincipal *,bool,nsISHEntry * *) [nsDocShell.cpp:07f70194e136 : 12440 + 0x14]
02:24:31     INFO -      eip = 0x03937fe4   esp = 0x0012f034   ebp = 0x0012f1d0
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -   8  xul.dll!nsDocShell::OnNewURI(nsIURI *,nsIChannel *,nsIPrincipal *,nsIPrincipal *,unsigned int,bool,bool,bool) [nsDocShell.cpp:07f70194e136 : 11734 + 0x24]
02:24:31     INFO -      eip = 0x03339676   esp = 0x0012f1d8   ebp = 0x0012f224
02:24:31     INFO -      Found by: previous frame's frame pointer
02:24:31     INFO -   9  xul.dll!nsDocShell::OnLoadingSite(nsIChannel *,bool,bool) [nsDocShell.cpp:07f70194e136 : 11805 + 0x19]
02:24:31     INFO -      eip = 0x03339b41   esp = 0x0012f22c   ebp = 0x0012f258
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  10  xul.dll!nsDocShell::CreateContentViewer(nsACString_internal const &,nsIRequest *,nsIStreamListener * *) [nsDocShell.cpp:07f70194e136 : 9147 + 0xc]
02:24:31     INFO -      eip = 0x033d4d3f   esp = 0x0012f260   ebp = 0x0012f288
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  11  xul.dll!nsDSURIContentListener::DoContent(nsACString_internal const &,bool,nsIRequest *,nsIStreamListener * *,bool *) [nsDSURIContentListener.cpp:07f70194e136 : 128 + 0x12]
02:24:31     INFO -      eip = 0x033d4aaa   esp = 0x0012f290   ebp = 0x0012f2b0
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  12  xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener *,nsIChannel *) [nsURILoader.cpp:07f70194e136 : 718 + 0x1c]
02:24:31     INFO -      eip = 0x033d49c4   esp = 0x0012f2b8   ebp = 0x0012f304
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  13  xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest *,nsISupports *) [nsURILoader.cpp:07f70194e136 : 396 + 0x17]
02:24:31     INFO -      eip = 0x033d4862   esp = 0x0012f30c   ebp = 0x0012f3d4
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  14  xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest *,nsISupports *) [nsURILoader.cpp:07f70194e136 : 259 + 0x9]
02:24:31     INFO -      eip = 0x033d43bd   esp = 0x0012f3dc   ebp = 0x0012f404
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  15  xul.dll!nsBaseChannel::OnStartRequest(nsIRequest *,nsISupports *) [nsBaseChannel.cpp:07f70194e136 : 809 + 0x16]
02:24:31     INFO -      eip = 0x033d3f6b   esp = 0x0012f40c   ebp = 0x0012f420
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  16  xul.dll!nsInputStreamPump::OnStateStart() [nsInputStreamPump.cpp:07f70194e136 : 524 + 0xd]
02:24:31     INFO -      eip = 0x034bdb08   esp = 0x0012f428   ebp = 0x0012f448
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  17  xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *) [nsInputStreamPump.cpp:07f70194e136 : 426 + 0x8]
02:24:31     INFO -      eip = 0x034bda3e   esp = 0x0012f450   ebp = 0x0012f460
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  18  xul.dll!nsInputStreamReadyEvent::Run() [nsStreamUtils.cpp:07f70194e136 : 95 + 0x7]
02:24:31     INFO -      eip = 0x034bd923   esp = 0x0012f468   ebp = 0x0012f474
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  19  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:07f70194e136 : 1082 + 0x6]
02:24:31     INFO -      eip = 0x033edfe9   esp = 0x0012f47c   ebp = 0x0012f504
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  20  xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [MessagePump.cpp:07f70194e136 : 96 + 0x16]
02:24:31     INFO -      eip = 0x033ec489   esp = 0x0012f50c   ebp = 0x0012f538
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  21  xul.dll!MessageLoop::RunHandler() [message_loop.cc:07f70194e136 : 225 + 0x8]
02:24:31     INFO -      eip = 0x03358f9d   esp = 0x0012f540   ebp = 0x0012f570
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  22  xul.dll!MessageLoop::Run() [message_loop.cc:07f70194e136 : 205 + 0x7]
02:24:31     INFO -      eip = 0x03358f6c   esp = 0x0012f578   ebp = 0x0012f590
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  23  xul.dll!nsBaseAppShell::Run() [nsBaseAppShell.cpp:07f70194e136 : 156 + 0x14]
02:24:31     INFO -      eip = 0x0366b76c   esp = 0x0012f598   ebp = 0x0012f5a0
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  24  xul.dll!nsAppShell::Run() [nsAppShell.cpp:07f70194e136 : 262 + 0x8]
02:24:31     INFO -      eip = 0x0366b4fb   esp = 0x0012f5a8   ebp = 0x0012f5b0
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  25  xul.dll!nsAppStartup::Run() [nsAppStartup.cpp:07f70194e136 : 283 + 0x6]
02:24:31     INFO -      eip = 0x0366b4b0   esp = 0x0012f5b8   ebp = 0x0012f5c4
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  26  xul.dll!XREMain::XRE_mainRun() [nsAppRunner.cpp:07f70194e136 : 4450 + 0xc]
02:24:31     INFO -      eip = 0x036352c1   esp = 0x0012f5cc   ebp = 0x0012f7b8
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  27  xul.dll!XREMain::XRE_main(int,char * * const,nsXREAppData const *) [nsAppRunner.cpp:07f70194e136 : 4583 + 0x7]
02:24:31     INFO -      eip = 0x03680555   esp = 0x0012f7c0   ebp = 0x0012f7e4
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  28  xul.dll!XRE_main [nsAppRunner.cpp:07f70194e136 : 4674 + 0x10]
02:24:31     INFO -      eip = 0x037853a5   esp = 0x0012f7ec   ebp = 0x0012f94c
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  29  firefox.exe!do_main [nsBrowserApp.cpp:07f70194e136 : 282 + 0x23]
02:24:31     INFO -      eip = 0x00401931   esp = 0x0012f954   ebp = 0x0012fbe4
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  30  firefox.exe!wmain [nsWindowsWMain.cpp:07f70194e136 : 115 + 0x2c2]
02:24:31     INFO -      eip = 0x004033b3   esp = 0x0012fbec   ebp = 0x0012ff78
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  31  firefox.exe!__scrt_common_main_seh [exe_common.inl : 253 + 0x1d]
02:24:31     INFO -      eip = 0x00405e37   esp = 0x0012ff80   ebp = 0x0012ffc0
02:24:31     INFO -      Found by: call frame info
02:24:31     INFO -  32  kernel32.dll!BaseProcessStart + 0x23
02:24:31     INFO -      eip = 0x7c817067   esp = 0x0012ffc8   ebp = 0x0012fff0
02:24:31     INFO -      Found by: call frame info

Comment 23

[Gary [:streetwolf52]]

Could this 'bad' patch cause Fx52 to crash when right clicking to bring up the context menu or clicking on the 'tools for all addons' button on the about:addons screen?  I'm asking cause you are backing this out and I don't want to report another bug if this one is the culprit.

Comment 24

[Carsten Book [:Tomcat]]

(In reply to Gary [:streetwolf] from comment #23)
> Could this 'bad' patch cause Fx52 to crash when right clicking to bring up
> the context menu or clicking on the 'tools for all addons' button on the
> about:addons screen?  I'm asking cause you are backing this out and I don't
> want to report another bug if this one is the culprit.

Hey Gary, this bug/crash never made it to mozilla-central (got backed out before) so i guess the crash you mentioned is maybe another bug

Comment 25

[Gary [:streetwolf52]]

I'm on inbound so I picked up this patch.  Btw... there was no crash dump produced.  I just got kicked out of Fx with the usual message.

Comment 26

[Gary [:streetwolf52]]

In safe-mode there are no crashes.  I'll try to identify the add-on.

Comment 27

[Gary [:streetwolf52]]

It turns it it is a userstyle I run via Stylish.  It changes the look of context menus so it makes sense. If it's not this patch then I'll figure out which patch is involved.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Gary, doesn't sound like this patch.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo if bug is intermittent) from comment #22)
> Backed out for crashing test with crash stacks involving sessionhistory and
> docshell:

After our discussion over IRC it's doubtful that this patch caused the issues described. Probably going to reland the patch because I really doubt that this patch is responsible for the problems.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51702f7d346f
Don't inherit principal for external loads and update documentation for principalToInherit. r=bz

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51702f7d346f