Open Bug 1321646 Opened 3 years ago Updated 2 years ago

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

Categories

(Firefox :: Private Browsing, defect, P3)

defect

Tracking

()

Tracking Status
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: huseby, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: stale-bug, Whiteboard: [OA])

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),
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) {
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.
Flags: needinfo?(tanvi)
Assignee: nobody → huseby
Flags: needinfo?(tanvi)
Priority: -- → P1
Assignee: huseby → nobody
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 '--'.
Keywords: stale-bug
FWIW, the bug here is caused by the BasePrincipal::Cast call. When I removed it, the crash went away.
Moving to p3 because no activity for at least 24 weeks.
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.