1321646 - Figure out why the nsDocShell::CopyFavicon assert fails and fix it

Status: NEW

(Firefox :: Private Browsing, defect, P3)

Description

[Dave Huseby [:huseby]]

Recently, Bug 1309070 was backed out because one of the asserts that was added keep tripping.  The repro steps are documented here: https://bgz.la/1320041#c4

The following diff is the assert that kept failing.  Investigation nodes can be found here: https://bgz.la/1309070#c29 What we're seeing is that the private browsing boolean flag isn't always the same as the private browsing origin attribute.  I'm not sure why this is the case, it *should* be the same, and if I'm wrong about that, then we need to know why it is OK that it is different.  Then we need to figure out how we can switch from the boolean to the origin attribute.  This bug will cover investigating and figuring this mystery out.

the diff: 


@@ -9548,16 +9552,19 @@ NS_IMPL_ISUPPORTS(nsCopyFaviconCallback, nsIFaviconDataCallback)
 } // namespace
 
 void
 nsDocShell::CopyFavicon(nsIURI* aOldURI,
                         nsIURI* aNewURI,
                         nsIPrincipal* aLoadingPrincipal,
                         bool aInPrivateBrowsing)
 {
+  MOZ_DIAGNOSTIC_ASSERT(
+    (BasePrincipal::Cast(aLoadingPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == aInPrivateBrowsing);
+
   if (XRE_IsContentProcess()) {
     dom::ContentChild* contentChild = dom::ContentChild::GetSingleton();
     if (contentChild) {
       mozilla::ipc::URIParams oldURI, newURI;
       SerializeURI(aOldURI, oldURI);
       SerializeURI(aNewURI, newURI);
       contentChild->SendCopyFavicon(oldURI, newURI,
                                     IPC::Principal(aLoadingPrincipal),

Comment 1

[Dave Huseby [:huseby]]

The assert in the following diff also fails with the repro from https://bgz.la/1320041#c4:

@@ -9506,16 +9508,18 @@ public:
                         nsIURI* aNewURI,
                         nsIPrincipal* aLoadingPrincipal,
                         bool aInPrivateBrowsing)
     : mSvc(aSvc)
     , mNewURI(aNewURI)
     , mLoadingPrincipal(aLoadingPrincipal)
     , mInPrivateBrowsing(aInPrivateBrowsing)
   {
+    MOZ_DIAGNOSTIC_ASSERT(
+      (BasePrincipal::Cast(aLoadingPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == aInPrivateBrowsing);
   }
 
   NS_IMETHOD
   OnComplete(nsIURI* aFaviconURI, uint32_t aDataLen,
              const uint8_t* aData, const nsACString& aMimeType) override
   {
     // Continue only if there is an associated favicon.
     if (!aFaviconURI) {

Comment 2

[Dave Huseby [:huseby]]

This another assert failure that is related to Bug 1320041.  Will you update this with a priority?  I think this is a P1.  If you agree, I'll pick this up.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.

Comment 4

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bugmasters/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 5

[Luke Crouch [:groovecoder]]

FWIW, the bug here is caused by the BasePrincipal::Cast call. When I removed it, the crash went away.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.