1309070 - check that chrome docshells never have private browsing mode set.

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P3)

Description

[Dave Huseby [:huseby]]

This bug is to add assertions that chrome docshells never have the private browsing origin attribute nor boolean set.

Comment 1

[Dave Huseby [:huseby]]

Notes from a recent contextual identities meeting:

    Check in docshell - check if private browsing mode flag in Origin Attribute is set correctly for Content docShells

    chrome docshell shouldn't have a private browser id set; but the xul page has an attribute that we can use to set in the content docshell.

    check that chrome docshells never have private browsing mode set.

    Addons have chrome docshells have addonids set; but chrome docshells should have default Origin Attributes

Comment 2

[Tanvi Vyas[:tanvi]]

Ehsan's response to comment 1; copied from https://bugzilla.mozilla.org/show_bug.cgi?id=1309067#c2 :


(In reply to Dave Huseby [:huseby] from comment #1)
> Notes from a recent contextual identities meeting:
> 
>     Check in docshell - check if private browsing mode flag in Origin
> Attribute is set correctly for Content docShells

Are the existing assertions there insufficient?
 
>     chrome docshell shouldn't have a private browser id set; but the xul
> page has an attribute that we can use to set in the content docshell.

I'm not sure if I understand this one...

>     check that chrome docshells never have private browsing mode set.

Again, are the existing assertions for this not sufficient?

>     Addons have chrome docshells have addonids set; but chrome docshells
> should have default Origin Attributes

Yeah, although I have a hard time imagining what the exact condition you're interested in is.  There's a chance that there are existing assertions covering that.

Comment 3

[Dave Huseby [:huseby]]

Attachment: adds diagnostic asserts for docshell private browsing checks.

I think this patch covers the only places we need to check that the private browsing id is 0.

Comment 4

[Dave Huseby [:huseby]]

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

Comment 5

[Dave Huseby [:huseby]]

so after digging into bug 1309067, i think the AssertOriginAttributesMatchPrivateBrowsing() assertion is sufficient but I think this bug should make calls to AssertOriginAttributesMatchPrivateBrowsing() instead of the manual asserts I added.

I'm going to add the two new calls, resubmit this patch and then close out 1309067 as won't fix because the existing asserts are sufficient.

Comment 6

[Dave Huseby [:huseby]]

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

Comment 7

[Dave Huseby [:huseby]]

Attachment: add a couple more calls to AssertOriginAttributesMatchPrivateBrowsing()

i think we should probably also add calls to AssertOriginAttributesMatchPrivateBrowsing() in the create and destroy functions so that we can cover the entire lifespan of docshells.

Comment 8

[Dave Huseby [:huseby]]

Ehsan isn't accepting any review requests until 11/10.  Can you take a look at this?  Thanks.

Comment 9

[Dave Huseby [:huseby]]

I was looking at nsDocShell.cpp tonight and noticed that the nsCopyFaviconCallback class takes both a loading principal and a boolean for "in private browsing".  Does it make sense to add an assertion there to check if the loading principal private browsing ID matches the aInPrivateBrowsing flag to the constructor?  I remember there was a few wrinkles in the way the origin attributes were managed in loading principals and I'm not sure what to do there.

Comment 10

[Dave Huseby [:huseby]]

Also, there's the nsDocShell::CopyFavicon function that takes aInPrivateBrowsing and loading context parameters.  Should I check the flag against the docshell origin attributes as well as the loading principal?

Comment 11

[Dave Huseby [:huseby]]

Hrm, so all of the calls to nsDocShell::CopyFavicon pass the NodePrincipal as the loading principal and then it passes in the docshell's private browsind ID origin attribute value.  So if anything, I think we should check if the flag matches the loading principal origin attributes.

Comment 12

[Dave Huseby [:huseby]]

maybe :baku knows the answer to my questions in comment 9, comment 10, and comment 11?

Comment 13

[(no longer active)]

(In reply to Dave Huseby [:huseby] from comment #9)
> I was looking at nsDocShell.cpp tonight and noticed that the
> nsCopyFaviconCallback class takes both a loading principal and a boolean for
> "in private browsing".  Does it make sense to add an assertion there to
> check if the loading principal private browsing ID matches the
> aInPrivateBrowsing flag to the constructor?  I remember there was a few
> wrinkles in the way the origin attributes were managed in loading principals
> and I'm not sure what to do there.

I don't think that assertion makes much sense.  It's trivial to check the only two consumers here:

<http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/docshell/base/nsDocShell.cpp#10428>
<http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/docshell/base/nsDocShell.cpp#12093>

Both of these are passing down the docshell's OA flag and the loading principals are those of the documents loaded inside the docshell, hence they'll also have the right OA.  I think we can just remove the boolean arguments from this code and use the principal directly.

Comment 14

[(no longer active)]

(In reply to Dave Huseby [:huseby] from comment #11)
> Hrm, so all of the calls to nsDocShell::CopyFavicon pass the NodePrincipal
> as the loading principal and then it passes in the docshell's private
> browsind ID origin attribute value.  So if anything, I think we should check
> if the flag matches the loading principal origin attributes.

You can perhaps do that alongside with comment 13.

Comment 15

[(no longer active)]

Comment on attachment 8804905 [details] [diff] [review]
add a couple more calls to AssertOriginAttributesMatchPrivateBrowsing()

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>index a25f87d..593da0f 100644
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -5725,25 +5725,29 @@ nsDocShell::Create()
> 
>   nsCOMPtr<nsIObserverService> serv = services::GetObserverService();
>   if (serv) {
>     const char* msg = mItemType == typeContent ?
>       NS_WEBNAVIGATION_CREATE : NS_CHROME_WEBNAVIGATION_CREATE;
>     serv->NotifyObservers(GetAsSupports(this), msg, nullptr);
>   }
> 
>+  AssertOriginAttributesMatchPrivateBrowsing();

Create() is called way too early, definitely before the docshell has been added to the docshell tree, and shortly after the nsDocShell object is created.  Given that we're asserting this in the constructor, why are you adding this assertion?

>+
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsDocShell::Destroy()
> {
>   NS_ASSERTION(mItemType == typeContent || mItemType == typeChrome,
>                "Unexpected item type in docshell");
> 
>+  AssertOriginAttributesMatchPrivateBrowsing();

This part looks good.

I also found that in SetOriginAttributes(), we assert this before but not after we're done computing the PB flag.  Can you also add this assertion to the end of that function as well while you're here?

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Dave Huseby [:huseby] from comment #12)
> maybe :baku knows the answer to my questions in comment 9, comment 10, and
> comment 11?

Tim worked on the favicon code, so moving the needinfo to him.  I would just add the assertions and then file a followup to remove the boolean in a few weeks.

Comment 17

[Dave Huseby [:huseby]]

re: comment 13

if you look at all of the callers of CopyFavicon, they are passing either a loading principal or the document node principal as well as the value of mIsPrivateBrowsing.

https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsDocShell%3A%3ACopyFavicon%28nsIURI+%2A%2C+nsIURI+%2A%2C+nsIPrincipal+%2A%2C+bool%29%22

I'm totally OK with the case where we're passing the loading principal because the loading principal is also what we use to initialize the origin attributes and the mIsPrivateBrowsing flag.  Where I get nervous is the calls to CopyFavicon that pass the document node principal.  Are those principals always the same as the loading principal used to initialize the docshell?  If so, then I agree, let's remove the boolean.  Otherwise, if we keep the boolean, we'll always be passing along the value from the loading context.

Comment 18

[Dave Huseby [:huseby]]

Attachment: Bug_1309070.patch

this is an updated patch with review feedback on the addition of AssertOriginAttributesMatchPrivateBrowsing() calls incorporated.

Comment 19

[(no longer active)]

(In reply to Dave Huseby [:huseby] from comment #17)
> re: comment 13
> 
> if you look at all of the callers of CopyFavicon, they are passing either a
> loading principal or the document node principal as well as the value of
> mIsPrivateBrowsing.
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=%2Bcallers%3A%22nsDocShell%3A%3ACopyFavicon%28nsIURI+%2A%2C+nsIURI+%
> 2A%2C+nsIPrincipal+%2A%2C+bool%29%22
> 
> I'm totally OK with the case where we're passing the loading principal
> because the loading principal is also what we use to initialize the origin
> attributes and the mIsPrivateBrowsing flag.  Where I get nervous is the
> calls to CopyFavicon that pass the document node principal.  Are those
> principals always the same as the loading principal used to initialize the
> docshell?

The principals are never the same, but the document inherits the *OA* on its principal from the docshell it is contained in.

> If so, then I agree, let's remove the boolean.  Otherwise, if we
> keep the boolean, we'll always be passing along the value from the loading
> context.

The ultimate goal is to remove the boolean flag *everywhere* it is currently passed.  I'm happy if you want to either do that directly or like comment 16.

Comment 20

[Dave Huseby [:huseby]]

Attachment: Bug_1309070.patch

Ehsan,

I updated the patch to add a couple more asserts related to the CopyFavicon code where we have both a principal and the PB boolean.  I will remove the boolean in a follow up bug.

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

Comment 21

[Dave Huseby [:huseby]]

filed bug 1319184 to handle the removal of the boolean.

Comment 22

[Tim Huang[:timhuang]]

I concur that we should add an assertion at CopyFavicon and remove the boolean in another bug.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbc7be933e2
make sure that docshell always has private browsing id unset. r=ehsan

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bbc7be933e2

Comment 25

[Andrew McCreight [:mccr8]]

Backed out for causing bug 1320041.

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

Comment 26

[Tanvi Vyas[:tanvi]]

What exactly is causing the crash in https://bugzilla.mozilla.org/show_bug.cgi?id=1320041?  The loadingPrincipal or OriginAttributes don't exist, or we actually have a mismatch of private browsing ids?

Comment 27

[Dave Huseby [:huseby]]

I'm doing a debug build with the original patch to check this out.

Comment 28

[Dave Huseby [:huseby]]

I am able to repro this crash on Ubuntu 16.04.1 amd64.  Investigating.

Comment 29

[Dave Huseby [:huseby]]

Alright, I finally got the crash.  The assert is failing.  I was able to repro it consistently using Kamil's repro from https://bgz.la/1320041#c4  Here's the callstack:

#0  0x00007fffe89a9d32 in nsDocShell::CopyFavicon(nsIURI*, nsIURI*, nsIPrincipal*, bool) (aOldURI=0x7fffc77593e0, aNewURI=0x7fffd99e6aa0, aLoadingPrincipal=0x7fffe14279e0, aInPrivateBrowsing=true) at /opt/gecko/gecko/docshell/base/nsDocShell.cpp:9564
#1  0x00007fffe898224c in nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString_internal const&, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (this=
    0x7fffc534a000, aURI=0x7fffd99e6aa0, aOriginalURI=0x0, aLoadReplace=false, aReferrer=0x0, aReferrerPolicy=1, aTriggeringPrincipal=0x7fffe14279e0, aPrincipalToInherit=0x0, aFlags=1, aWindowTarget=..., aTypeHint=0x0, aFileName=..., aPostData=0x0, aHeadersData=0x0, aLoadType=134217729, aSHEntry=
    0x0, aFirstParty=true, aSrcdoc=..., aSourceDocShell=0x7fffc534a1a8, aBaseURI=0x0, aDocShell=0x0, aRequest=0x0) at /opt/gecko/gecko/docshell/base/nsDocShell.cpp:10448
#2  0x00007fffe897d5c0 in nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) (this=0x7fffc534a000, aURI=0x7fffd99e6aa0, aLoadInfo=0x7fffd99e6b30, aLoadFlags=0, aFirstParty=true) at /opt/gecko/gecko/docshell/base/nsDocShell.cpp:1558
#3  0x00007fffe5abd2dd in mozilla::dom::Location::SetURI(nsIURI*, bool) (this=0x7fffc6fc6c40, aURI=0x7fffd99e6aa0, aReplace=false) at /opt/gecko/gecko/dom/base/Location.cpp:288
#4  0x00007fffe5abd895 in mozilla::dom::Location::SetHash(nsAString_internal const&) (this=0x7fffc6fc6c40, aHash=...) at /opt/gecko/gecko/dom/base/Location.cpp:367
#5  0x00007fffe6b5e0c2 in mozilla::dom::Location::SetHash(nsAString_internal const&, nsIPrincipal&, mozilla::ErrorResult&) (this=0x7fffc6fc6c40, aHash=..., aSubjectPrincipal=..., aError=...)
    at /opt/gecko/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/Location.h:209
#6  0x00007fffe6b3c5b5 in mozilla::dom::LocationBinding::set_hash(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Location*, JSJitSetterCallArgs) (cx=0x7fffde504000, obj=(JSObject * const) 0x7fffb4272d40 [object Location], self=0x7fffc6fc6c40, args=$jsval("privacy")) at /opt/gecko/gecko/obj-x86_64-pc-linux-gnu/dom/bindings/LocationBinding.cpp:703
#7  0x00007fffe6b77b58 in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) (cx=0x7fffde504000, argc=1, vp=0x7fffffff61b0)
    at /opt/gecko/gecko/dom/bindings/BindingUtils.cpp:2847
#8  0x00007fffea92e86d in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x7fffde504000, native=0x7fffe6b778c0 <mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*)>, args=...) at /opt/gecko/gecko/js/src/jscntxtinlines.h:239
#9  0x00007fffea903d10 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (cx=0x7fffde504000, args=..., construct=js::NO_CONSTRUCT)
    at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:457
#10 0x00007fffea9041cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) (cx=0x7fffde504000, args=...) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:502
#11 0x00007fffea904246 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (cx=0x7fffde504000, fval=$jsval((JSObject *) 0x7fffb42a2680 [object Function "set hash"]), thisv=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), args=..., rval=JSVAL_VOID) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:521
#12 0x00007fffea904dcb in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) (cx=0x7fffde504000, thisv=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), setter=$jsval((JSObject *) 0x7fffb42a2680 [object Function "set hash"]), v=$jsval("privacy")) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:648
#13 0x00007fffea7523ea in js::SetPropertyIgnoringNamedGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) (cx=0x7fffde504000, obj=(JSObject * const) 0x7fffb4272d40 [object Location], id=$jsid("hash"), v=$jsval("privacy"), receiver=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), ownDesc_={obj = (JSObject *) 0x7fffb4272d40 [object Location], attrs = 117, getter = 0x7fffb42a2640, setter = 0x7fffb42a2680, value = JSVAL_VOID}, result=...)
    at /opt/gecko/gecko/js/src/proxy/BaseProxyHandler.cpp:245
#14 0x00007fffe6b7bea2 in mozilla::dom::DOMProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) const (this=0x7fffed5d1a50 <mozilla::dom::LocationBinding::DOMProxyHandler::getInstance()::instance>, cx=0x7fffde504000, proxy=(JSObject * const) 0x7fffb4272d40 [object Location], id=$jsid("hash"), v=$jsval("privacy"), receiver=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), result=...) at /opt/gecko/gecko/dom/bindings/DOMJSProxyHandler.cpp:258
#15 0x00007fffea777085 in js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (cx=0x7fffde504000, proxy=(JSObject * const) 0x7fffb4272d40 [object Location], id=$jsid("hash"), v=$jsval("privacy"), receiver_=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), result=...)
    at /opt/gecko/gecko/js/src/proxy/Proxy.cpp:333
#16 0x00007fffea77887d in js::proxy_SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (cx=0x7fffde504000, obj=(JSObject * const) 0x7fffb4272d40 [object Location], id=$jsid("hash"), v=$jsval("privacy"), receiver=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), result=...)
    at /opt/gecko/gecko/js/src/proxy/Proxy.cpp:589
#17 0x00007fffea6cfb70 in JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (cx=0x7fffde504000, obj=(JSObject * const) 0x7fffb4272d40 [object Location], id=$jsid("hash"), v=$jsval("privacy"), receiver=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), result=...)
    at /opt/gecko/gecko/js/src/jsobj.cpp:1019
#18 0x00007fffea038ffe in js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) (cx=0x7fffde504000, obj=(JSObject * const) 0x7fffb4272d40 [object Location], id=$jsid("hash"), v=$jsval("privacy"), receiver=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), result=...)
    at /opt/gecko/gecko/js/src/vm/NativeObject.h:1539
#19 0x00007fffea925fd0 in SetPropertyOperation(JSContext*, JSOp, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::Handle<JS::Value>) (cx=0x7fffde504000, op=JSOP_STRICTSETPROP, lval=$jsval((JSObject *) 0x7fffb4272d40 [object Location]), id=$jsid("hash"), rval=$jsval("privacy")) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:259
#20 0x00007fffea8f6de7 in Interpret(JSContext*, js::RunState&) (cx=0x7fffde504000, state=...) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:2713
#21 0x00007fffea8edff9 in js::RunScript(JSContext*, js::RunState&) (cx=0x7fffde504000, state=...) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:403
#22 0x00007fffea903dfa in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (cx=0x7fffde504000, args=..., construct=js::NO_CONSTRUCT)
    at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:475
#23 0x00007fffea9041cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) (cx=0x7fffde504000, args=...) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:502
#24 0x00007fffea904246 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (cx=0x7fffde504000, fval=$jsval((JSObject *) 0x7fffc568d100 [object Function "onLoad"]), thisv=$jsval((JSObject *) 0x7fffb34cd120 [object Proxy]), args=..., rval=JSVAL_VOID) at /opt/gecko/gecko/js/src/vm/Interpreter.cpp:521
#25 0x00007fffea5e5b80 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (cx=0x7fffde504000, thisv=$jsval((JSObject *) 0x7fffb34cd120 [object Proxy]), fval=$jsval((JSObject *) 0x7fffc568d100 [object Function "onLoad"]), args=..., rval=JSVAL_VOID) at /opt/gecko/gecko/js/src/jsapi.cpp:2830
#26 0x00007fffe6823c9b in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) (this=0x7fffc775bf80, cx=0x7fffde504000, aThisVal=$jsval((JSObject *) 0x7fffb34cd120 [object Proxy]), event=..., aRv=...) at /opt/gecko/gecko/obj-x86_64-pc-linux-gnu/dom/bindings/EventListenerBinding.cpp:47
#27 0x00007fffe6e4c86f in mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) (this=0x7fffc775bf80, thisVal=@0x7fffffffa3e0: 0x7fffd3a32000, event=..., aRv=..., aExecutionReason=0x7fffeb1b5bbf "EventListener.handleEvent", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aCompartment=0x0) at /opt/gecko/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/EventListenerBinding.h:64
#28 0x00007fffe6e2d063 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) (this=0x7fffc6090310, aListener=0x7fffc6090340, aDOMEvent=0x7fffc87591c0, aCurrentTarget=0x7fffd3a32000) at /opt/gecko/gecko/dom/events/EventListenerManager.cpp:1129
#29 0x00007fffe6e2d7ce in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (this=0x7fffc6090310, aPresContext=0x7fffd2f35000, aEvent=0x7fffc87bf260, aDOMEvent=0x7fffffffa950, aCurrentTarget=0x7fffd3a32000, aEventStatus=0x7fffffffa958)
    at /opt/gecko/gecko/dom/events/EventListenerManager.cpp:1286
#30 0x00007fffe6e5786d in mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (this=0x7fffc6090310, aPresContext=0x7fffd2f35000, aEvent=0x7fffc87bf260, aDOMEvent=0x7fffffffa950, aCurrentTarget=0x7fffd3a32000, aEventStatus=0x7fffffffa958)
    at /opt/gecko/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EventListenerManager.h:374
#31 0x00007fffe6e4a508 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) (this=0x7fffca781030, aVisitor=..., aCd=...)
    at /opt/gecko/gecko/dom/events/EventDispatcher.cpp:314
#32 0x00007fffe6e2515f in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (aChain=nsTArray<mozilla::EventTargetChainItem> & = {...}, aVisitor=..., aCallback=0x0, aCd=...) at /opt/gecko/gecko/dom/events/EventDispatcher.cpp:487
#33 0x00007fffe6e265ff in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=0x7fffc704e000, aPresContext=0x7fffd2f35000, aEvent=0x7fffc87bf260, aDOMEvent=0x7fffc87591c0, aEventStatus=0x7fffffffabdc, aCallback=0x0, aTargets=0x0)
    at /opt/gecko/gecko/dom/events/EventDispatcher.cpp:820
#34 0x00007fffe6e26b79 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (aTarget=0x7fffc704e000, aEvent=0x0, aDOMEvent=0x7fffc87591c0, aPresContext=0x7fffd2f35000, aEventStatus=0x7fffffffabdc) at /opt/gecko/gecko/dom/events/EventDispatcher.cpp:886
#35 0x00007fffe5c50095 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) (this=0x7fffc704e000, aEvent=0x7fffc87591c0, aRetVal=0x7fffffffaca3) at /opt/gecko/gecko/dom/base/nsINode.cpp:1298
#36 0x00007fffe594c16b in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*, bool) (aDoc=
    0x7fffc704e000, aTarget=0x7fffc704e000, aEventName=..., aCanBubble=true, aCancelable=false, aTrusted=true, aDefaultAction=0x0, aOnlyChromeDispatch=false)
    at /opt/gecko/gecko/dom/base/nsContentUtils.cpp:4026
#37 0x00007fffe594beb6 in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) (aDoc=0x7fffc704e000, aTarget=0x7fffc704e000, aEventName=..., aCanBubble=true, aCancelable=false, aDefaultAction=0x0) at /opt/gecko/gecko/dom/base/nsContentUtils.cpp:3994
#38 0x00007fffe5bae3d6 in nsDocument::DispatchContentLoadedEvents() (this=0x7fffc704e000) at /opt/gecko/gecko/dom/base/nsDocument.cpp:4961
#39 0x00007fffe7bb3281 in mozilla::dom::XULDocument::DoneWalking() (this=0x7fffc704e000) at /opt/gecko/gecko/dom/xul/XULDocument.cpp:3037
#40 0x00007fffe7ba85e0 in mozilla::dom::XULDocument::ResumeWalk() (this=0x7fffc704e000) at /opt/gecko/gecko/dom/xul/XULDocument.cpp:2964
#41 0x00007fffe7bb477a in mozilla::dom::XULDocument::OnScriptCompileComplete(JSScript*, nsresult) (this=0x7fffc704e000, aScript=0x7fffb3d5a120, aStatus=nsresult::NS_OK)
    at /opt/gecko/gecko/dom/xul/XULDocument.cpp:3458
#42 0x00007fffe7bcbfc8 in NotifyOffThreadScriptCompletedRunnable::Run() (this=0x7fffc7216520) at /opt/gecko/gecko/dom/xul/nsXULElement.cpp:2814
#43 0x00007fffe3e1c098 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fffe1421300, aMayWait=true, aResult=0x7fffffffb7fe) at /opt/gecko/gecko/xpcom/threads/nsThread.cpp:1213
#44 0x00007fffe3e9ab5c in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fffe1421300, aMayWait=true) at /opt/gecko/gecko/xpcom/glue/nsThreadUtils.cpp:361
#45 0x00007fffe470d9a9 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7fffe142d680, aDelegate=0x7ffff6bb1690) at /opt/gecko/gecko/ipc/glue/MessagePump.cpp:124
#46 0x00007fffe466c305 in MessageLoop::RunInternal() (this=0x7ffff6bb1690) at /opt/gecko/gecko/ipc/chromium/src/base/message_loop.cc:232
#47 0x00007fffe466c285 in MessageLoop::RunHandler() (this=0x7ffff6bb1690) at /opt/gecko/gecko/ipc/chromium/src/base/message_loop.cc:225
#48 0x00007fffe466c25d in MessageLoop::Run() (this=0x7ffff6bb1690) at /opt/gecko/gecko/ipc/chromium/src/base/message_loop.cc:205
#49 0x00007fffe7d61e93 in nsBaseAppShell::Run() (this=0x7fffd784e470) at /opt/gecko/gecko/widget/nsBaseAppShell.cpp:156
#50 0x00007fffe8f1a342 in nsAppStartup::Run() (this=0x7fffd78519c0) at /opt/gecko/gecko/toolkit/components/startup/nsAppStartup.cpp:283
#51 0x00007fffe9012f57 in XREMain::XRE_mainRun() (this=0x7fffffffc1d8) at /opt/gecko/gecko/toolkit/xre/nsAppRunner.cpp:4480
#52 0x00007fffe9013a36 in XREMain::XRE_main(int, char**, nsXREAppData const*) (this=0x7fffffffc1d8, argc=4, argv=0x7fffffffd6b8, aAppData=0x7fffffffc498)
    at /opt/gecko/gecko/toolkit/xre/nsAppRunner.cpp:4613
#53 0x00007fffe901420f in XRE_main(int, char**, nsXREAppData const*, uint32_t) (argc=4, argv=0x7fffffffd6b8, aAppData=0x7fffffffc498, aFlags=0)
    at /opt/gecko/gecko/toolkit/xre/nsAppRunner.cpp:4704
#54 0x0000000000405c5f in do_main(int, char**, char**, nsIFile*) (argc=4, argv=0x7fffffffd6b8, envp=0x7fffffffd6e0, xreDirectory=0x7ffff6b5fb40)
    at /opt/gecko/gecko/browser/app/nsBrowserApp.cpp:328
#55 0x0000000000405382 in main(int, char**, char**) (argc=4, argv=0x7fffffffd6b8, envp=0x7fffffffd6e0) at /opt/gecko/gecko/browser/app/nsBrowserApp.cpp:461


Alright, so I'm pretty sure the problem comes from the fact that about:debugging is mType = nsIDocument::eXUL which means that the principal returned from doc->NodePrincipal() is going to have a private browsing id of 0 since it inherits from chrome origin attributes.  But since this is a private browsing windows, the aInPrivateBrowsing boolean will be true, which is what I'm seeing:

9560      BasePrincipal * bp = BasePrincipal::Cast(aLoadingPrincipal);
9561      OriginAttributes const & attrs = bp->OriginAttributesRef();
9562      int const bpid = attrs.mPrivateBrowsingId;

(gdb) p bpid
$11 = 0
(gdb) p aInPrivateBrowsing
$12 = true
(gdb)

I think the correct fix would be to push this assert out to the call site of CopyFavicon instead of being inside of CopyFavicon.  Then I can test the type of the doc/docshell and decide if it makes sense to compare the origin attribute to the boolean value.

Comment 30

[Dave Huseby [:huseby]]

Attachment: Bug_1309070.2.patch removed the problematic asserts and relanding

this is the same patch that was r+ before but i removed the asserts that caused it to be backed out.

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

Comment 31

[Dave Huseby [:huseby]]

I filed Bug 1321646 to track the work figuring out what is up with the failing asserts.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa1300f064d
make sure that docshell always has private browsing id unset. r=ehsan

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9fa1300f064d