Closed Bug 1335678 Opened 3 years ago Closed 3 years ago

Add diagnostic assertions to see if OriginAttributes and loadGroup are in sync

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 + fixed

People

(Reporter: baku, Assigned: baku)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [tor][domsecurity-backlog1][OA])

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1302566 +++
Attached patch revert.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8832374 - Flags: review?(ehsan)
Comment on attachment 8832374 [details] [diff] [review]
revert.patch

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

::: dom/base/nsContentUtils.cpp
@@ +1812,5 @@
> +    return isPrivate;
> +  }
> +
> +  nsCOMPtr<nsIChannel> channel = aDoc->GetChannel();
> +  return channel && NS_UsePrivateBrowsing(channel);

It's not really great that we're now exposing a different way of getting this info from nsContentUtils.

I think what you want instead is to make this a private static function, and expose a VerifyDocumentPrivateBrowsingState() that will compute the information both using the OA from the principal and the helper mentioned above, and asserts they're the same.
Attachment #8832374 - Flags: review?(ehsan) → review-
[Tracking Requested - why for this release]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1302566 landed in Firefox 54.  We should have the assertions that validate that that conversion happens properly in the same release.  Otherwise, we risk introducing a bug in private mode / regular mode separation.
Attached patch revert.patch (obsolete) — Splinter Review
Attachment #8832374 - Attachment is obsolete: true
Attachment #8832833 - Flags: review?(ehsan)
Attachment #8832833 - Flags: review?(ehsan) → review+
baku said when he pushed this to try, he got lots of failures.  So he is going to back out https://bugzilla.mozilla.org/show_bug.cgi?id=1302566 after all.
Tracking 54+ for the reasons in Comment 3.
Attached patch revert_a.patchSplinter Review
The previous patch broke everything. We need to revert the commit and see why there are so many failures with these assertions.
Attachment #8832833 - Attachment is obsolete: true
Attachment #8833918 - Flags: review?(ehsan)
Comment on attachment 8833918 [details] [diff] [review]
revert_a.patch

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

Sadface :(
Attachment #8833918 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/7c00dba4cfdf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1337055
You need to log in before you can comment on or make changes to this bug.