1281183 - Remove Notification PB Flags

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[James Andreou]

Removing the old private browsing flags being passed around the alert notification system and use the value from Origin Attributes.

Comment 1

[James Andreou]

Attachment: Bug1281183.patch

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 8764018 [details] [diff] [review]
Bug1281183.patch

Review of attachment 8764018 [details] [diff] [review]:
-----------------------------------------------------------------

These changes generally make sense. I'm concerned about modifying public interfaces and compatibility with old download databases, however.

::: toolkit/components/alerts/nsIAlertsService.idl
@@ +26,4 @@
>              [optional] in AString dir,
>              [optional] in AString lang,
>              [optional] in AString data,
> +            [optional] in nsIPrincipal principal);

Note to self: we need to check whether these interfaces are used by addons before landing this change. That requires bug 1280948 to be fixed.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +1267,5 @@
>    }
>  
> +  nsCString originSuffix;
> +  stmt->GetUTF8String(i++, originSuffix);
> +  dl->mPrincipal = mozilla::BasePrincipal::CreateCodebasePrincipal(originSuffix);

What happens when opening up a DB from a version of Gecko before this change?

@@ +1631,5 @@
>      (void)aMIMEInfo->GetPreferredAction(&action);
>    }
>  
> +  nsAutoCString originSuffix;
> +  rv = aPrincipal->GetOrigin(originSuffix);

The naming here doesn't match what we're actually getting - we're grabbing a full origin + suffix from the principal, but the names imply that we're only storing the suffix.

::: uriloader/base/nsITransfer.idl
@@ +60,5 @@
>                in PRTime startTime,
>                in nsIFile aTempFile,
>                in nsICancelable aCancelable,
> +              in boolean aIsPrivate,
> +              in nsIPrincipal aPrincipal);

Note to self: check whether this interface is used by addons.

::: widget/cocoa/OSXNotificationCenter.mm
@@ +400,1 @@
>                                        : imgLoader::NormalLoader();

nit: align this with the ?

Comment 3

[(no longer active)]

Michael, can you please help drive this to the finish line?  Thanks!

Comment 4

[Dave Huseby [:huseby]]

I just completed a security inspection of the proposed patch and it looks correct.  I even audited the custom serialization protocol for the AlertNotificationType.  The switch from private browsing flag to origin attributes looks clean to me.

Comment 5

[Dave Huseby [:huseby]]

One more note: the parent side code used to always grab the private browsing flag from the parent-side doc rather than trust the value from the child.  that code was replaced with code that grabbed the principal from the parent-side doc instead of trusting the value from the child.

Comment 6

[Dave Huseby [:huseby]]

Tanvi, I went through this code looking for places to add assertions but it has been completely converted to origin attributes and as I stated in comment 4 and comment 5, the parent-side origin attributes are taking from the window doc instead of the IPC messages so the enforcement appears to be correct for me.  I couldn't see any obvious places to put assertions that would ensure that booleans and the origin attributes are matching since the conversion to origin attributes is complete in this part of the code.

Comment 7

[Tanvi Vyas[:tanvi]]

Okay, so sounds like this bug doesn't need an assertion because both the previous boolean and the new way of getting private browsing are both coming from the loadcontext OAs.

Comment 8

[Dave Huseby [:huseby]]

Tanvi, to be clear, we agree, no assertions need to be added.  This patch is fine by me.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I don't think I will be able to get around to this anytime soon.

Comment 10

[Andrew Overholt [:overholt]]

Maybe baku wants to take this at some point (I'll ask him). I'm going to mark this as P2 since it's not a super-immediate-going-to-happen-this-cycle piece of work.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information