1309067 - add assertions that compare the private browsing boolean and the private browsing OriginAttribute for places that have not been converted

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Dave Huseby [:huseby]]

We need to convince ourselves that the private browser origin attribute correctly matches the private browsing boolean flag that it is replaces.  This bug is to add a bunch of assertions in the code to make sure that the origin attribute matches the boolean.

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

[(no longer active)]

(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]]

Ehsan, disregard Comment 1, it's the wrong bug.  That was intended for Bug 1309070.

Comment 4

[Dave Huseby [:huseby]]

The plan for this bug has shifted to be one that adds diagnostic asserts to nsDocShell, per the discussion on origin attributes invariants.  The notes are here: https://public.etherpad-mozilla.org/p/PrivateBrowsingInvariants

Comment 5

[Dave Huseby [:huseby]]

The existing AssertOriginAttributesMatchPrivateBrowsing() is sufficient for asserting this in the docshell.  I'm resolving this as won't fix.  See bug 1309070 for a couple more places I added calls to the function.

Comment 6

[Dave Huseby [:huseby]]

whoops, this is for every place else other than docshell.

Comment 7

[Dave Huseby [:huseby]]

Attachment: WIP patch adds a couple of more asserts.

I went through the entire list in this query:

https://dxr.mozilla.org/mozilla-central/search?q=PrivateBrowsing+ext%3Acpp&redirect=true

and I was only able to find a couple places where we have the old boolean and the new origin attributes available.  this patch is a WIP until I can get a clobber build and try push through.

Comment 8

[Dave Huseby [:huseby]]

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

Comment 9

[Dave Huseby [:huseby]]

Attachment: Bug_1309067.patch

After reviewing all of the PrivateBrowsing instances in the cpp code, these are the only asserts I found to add.

Comment 10

[(no longer active)]

Comment on attachment 8811536 [details] [diff] [review]
Bug_1309067.patch

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

::: dom/indexedDB/IDBFactory.cpp
@@ +611,5 @@
> +    {
> +      uint32_t privateBrowsingId = 0;
> +      aPrincipal->GetPrivateBrowsingId(&privateBrowsingId);
> +      MOZ_DIAGNOSTIC_ASSERT(mPrivateBrowsingMode == (privateBrowsingId > 0));
> +    }

This can be written much more simply as:

  MOZ_DIAGNOSTIC_ASSERT(mPrivateBrowsingMode == (aPrincipal->GetPrivateBrowsingId() > 0));

Also please remove the nested scope.

Comment 11

[Dave Huseby [:huseby]]

Attachment: Bug_1309067.patch

Updated patch with review feedback. r+ ehsan

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c21850fc2d08
adding asserts to ensure boolean and OA private browsing values match. r=ehsan

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c21850fc2d08

Comment 14

[areya]

https://hg.mozilla.org/mozilla-central/rev/c21850fc2d08

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Is there another bug tracking failed assertions from this bug? (e.g. this crash bp-38dfc043-6b36-4b4e-9998-d26ee2170223)

Comment 16

[Dave Huseby [:huseby]]

Not that I know of.