check that chrome docshells never have private browsing mode set.

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [OA][domsecurity-active])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
This bug is to add assertions that chrome docshells never have the private browsing origin attribute nor boolean set.
(Assignee)

Updated

2 years ago
Assignee: nobody → huseby
(Assignee)

Comment 1

2 years ago
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

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8804407 [details] [diff] [review]
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.
Attachment #8804407 - Flags: feedback?(tanvi)
(Assignee)

Updated

2 years ago
Attachment #8804407 - Flags: feedback?(tanvi)
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8804905 [details] [diff] [review]
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.
Attachment #8804407 - Attachment is obsolete: true
Attachment #8804905 - Flags: review?(amarchesini)
(Assignee)

Comment 8

2 years ago
Ehsan isn't accepting any review requests until 11/10.  Can you take a look at this?  Thanks.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8804905 - Flags: review?(amarchesini) → review?(ehsan)
(Assignee)

Comment 9

2 years ago
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.
Flags: needinfo?(tanvi)
Flags: needinfo?(ehsan)
(Assignee)

Comment 10

2 years ago
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?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

2 years ago
maybe :baku knows the answer to my questions in comment 9, comment 10, and comment 11?
Flags: needinfo?(amarchesini)

Comment 13

2 years ago
(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.
Flags: needinfo?(ehsan)

Comment 14

2 years ago
(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

2 years ago
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?
Attachment #8804905 - Flags: review?(ehsan) → review-

Comment 16

2 years ago
(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.
Flags: needinfo?(tihuang)
Flags: needinfo?(tanvi)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 17

2 years ago
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 18

2 years ago
Created attachment 8812390 [details] [diff] [review]
Bug_1309070.patch

this is an updated patch with review feedback on the addition of AssertOriginAttributesMatchPrivateBrowsing() calls incorporated.
Attachment #8804905 - Attachment is obsolete: true
Attachment #8812390 - Flags: review?(ehsan)

Comment 19

2 years ago
(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.
Flags: needinfo?(ehsan)

Updated

2 years ago
Attachment #8812390 - Flags: review?(ehsan) → review+
(Assignee)

Comment 20

2 years ago
Created attachment 8812899 [details] [diff] [review]
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
Attachment #8812390 - Attachment is obsolete: true
Attachment #8812899 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Blocks: 1319184
(Assignee)

Comment 21

2 years ago
filed bug 1319184 to handle the removal of the boolean.

Updated

2 years ago
Attachment #8812899 - Flags: review?(ehsan) → review+
I concur that we should add an assertion at CopyFavicon and remove the boolean in another bug.
Flags: needinfo?(tihuang)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 23

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbc7be933e2
make sure that docshell always has private browsing id unset. r=ehsan
Keywords: checkin-needed

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2bbc7be933e2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1320041
Backed out for causing bug 1320041.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dec2a039683fe1c0588bbee14817c5416d692fa1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox53: fixed → ---
Target Milestone: Firefox 53 → ---

Comment 26

2 years ago
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?
(Assignee)

Comment 27

2 years ago
I'm doing a debug build with the original patch to check this out.
(Assignee)

Comment 28

2 years ago
I am able to repro this crash on Ubuntu 16.04.1 amd64.  Investigating.
(Assignee)

Comment 29

2 years ago
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.
Flags: needinfo?(tanvi)
(Assignee)

Updated

2 years ago
See Also: → bug 1321646
(Assignee)

Comment 30

2 years ago
Created attachment 8816282 [details] [diff] [review]
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
Attachment #8812899 - Attachment is obsolete: true
Flags: needinfo?(tanvi)
Attachment #8816282 - Flags: review+
(Assignee)

Comment 31

2 years ago
I filed Bug 1321646 to track the work figuring out what is up with the failing asserts.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 32

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa1300f064d
make sure that docshell always has private browsing id unset. r=ehsan
Keywords: checkin-needed

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9fa1300f064d
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Updated

2 years ago
Blocks: 1323262
You need to log in before you can comment on or make changes to this bug.