1322760 - investigate why the private browsing boolean and private browsing origin ID aren't always the same in global window

Status: RESOLVED DUPLICATE of bug 1319951

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

Description

[Dave Huseby [:huseby]]

Bug 1321969 came up because an assert added in Bug 1316075 caused a spike in crashes.  This bug is for finding a test case and to better understand why the assert was being triggered.

Comment 1

[Dave Huseby [:huseby]]

Will you give me a priority on this please?  Thanks!

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:
We relaxed a diagnostic assert that was triggering in FF53.  We need to figure out what went wrong here before FF53 ships.

Comment 3

[Tanvi Vyas[:tanvi]]

So if I understand correctly, the assertion added here - https://bugzilla.mozilla.org/attachment.cgi?id=8815588&action=diff#a/dom/base/nsGlobalWindow.cpp_sec4 - in bug 1316075 fired and caused crashes (bug 1321969).  The assertion was added because we previously converted that call site to use the Origin Attribute private browsing mode flag instead of the boolean (in bug 1276328) and wanted to make sure that we didn't make a mistake.  Bug 1276328 landed in Firefox 50.

We need to figure out why we have a mismatch, and which value is right - the Origin Attribute or the previous boolean.  Then we need to change the behavior accordingly.  If the behavior of the OA is wrong and the boolean was right, then we need to switch back to the boolean and uplift up as far as we can.  If the behavior of the boolean was wrong, then we started using the right value with Firefox 50+.

Given this is in "GetLocalStorage", we may end up leaking across private mode and regular mode if we don't have the right value.

Comment 4

[Marcia Knous [:marcia]]

Tracking 53+ for the reason in Comment 2.

Comment 5

[Dave Huseby [:huseby]]

Attachment: patch to restore the assert for testing.

this patch restores the assert that was causing the crashes.

I have solid repro steps:

0. apply the patch to nightly.
1. enable private browsing autostart.
2. install the Blur addon.

crash.  I am not unstuck.

Comment 6

[Dave Huseby [:huseby]]

I am *now* unstuck.

Comment 7

[Dave Huseby [:huseby]]

Looking at the crash in visual studio reveals that on startup, the PB origin attribute is 0, but the IsPrivateBrowsing() function returns true because PB autostart is enabled.

Comment 8

[(no longer active)]

Dave is away right now, I'll have a look at this later today.

Comment 9

[(no longer active)]

First interesting fact, looking at <https://crash-stats.mozilla.com/signature/?product=Firefox&version=53.0a1&signature=nsGlobalWindow::GetLocalStorage&date=%3E%3D2016-12-02T14:48:24.000Z&date=%3C2016-12-03T14:48:24.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#correlations>, this crash is correlated with a number of add-ons, including Blur.

Comment 10

[(no longer active)]

OK, the problem here is the same thing that I described back in bug 1316075 comment 17 (item #2 there) which we filed bug 1319951 for.  Here we simply have a system principal with pbId==0 and we're comparing that against browser.xul's docshell.

I don't think there's any point in having more than one bug for this, so duping.

Comment 12

[(no longer active)]

Bug 1319951 is already tracking-firefox53+.