1319908 - Opening bookmarks menu trips mPrivateBrowsingId assertion when browser is set to "Never Remember History"

Status: RESOLVED FIXED

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

Description

[Lina Butler [:lina]]

STR:

1. In a debug build, open about:preferences#privacy.
2. Under "History", change the setting to "Nightly will: Never remember history" and restart.
3. Open the bookmarks menu.

That will trip this assertion: http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/netwerk/base/nsNetUtil.cpp#2369-2372
Stack: https://gist.github.com/kitcambridge/7bdcea694a7e4704c65f4b39e2e943b4

Comment 1

[Tanvi Vyas[:tanvi]]

Do we have a regression range?

Comment 2

[Kamil Jozwiak [:kjozwiak]]

I went through mozregression [1] and it seems like the issue has been present in nightly since:

0:11.06 INFO: application_buildid: 20160927153519
0:11.06 INFO: application_changeset: b82da770e632654ed0b001a1fa4e4986c8be4f74
0:11.06 INFO: application_name: Firefox
0:11.06 INFO: application_repository: https://hg.mozilla.org/integration/mozilla-inbound
0:11.06 INFO: application_version: 52.0a1

This was the closest regression range that I could generate. There's several inbound builds within that range that crash fx instantly after you restart the browser when selecting "Never remember history" under about:preferences#privacy. I'm assuming it has something to do with this issue.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fa29485efec0edaae7444ed651982ce5c3beeb95&tochange=5285464565a804f3766f273b20fb7147f92db53e

[1] mozregression --repo mozilla-inbound --build-type debug --good 2016-09-26 --bad 2016-09-27

Comment 3

[Tanvi Vyas[:tanvi]]

This is likely the culprit, but should be confirmed:

[e-mail]
Tue Sep 27 20:57:06 2016 +0000	31c6da3081bf	James Andreou — Bug 1282124 - Remove nsILoadInfo.usePrivateBrowsing and the SEC_FORCE_PRIVATE_BROWSING flag; r=smaug,jryans

https://hg.mozilla.org/integration/mozilla-inbound/rev/31c6da3081bf

Comment 4

[Dave Huseby [:huseby]]

I was able to repro the crash on changeset c9e65e3bf84a which is the one before 31c6da3081bf, Bug 1282124.

Comment 5

[Dave Huseby [:huseby]]

I bisected this and came up with revision 7bcb0c1694 as the first bad commit.

Comment 6

[(no longer active)]

I'm going to take a look at this since it was me who landed the offending patch.

Comment 7

[(no longer active)]

It seems like this is more of a "regression" from bug 1277803 than anything.  The regression is in quotes here because there's nothing wrong with what's happening, it's just that we need to disable this assertion.

Bug 1277803 special cased this assertion for the case of favicons loaded through a <xul:image>: <https://hg.mozilla.org/mozilla-central/rev/ae9d0ae4bba4>.  However it turns out that on OSX we load favicons for things such as the Bookmarks menu by looking at the image attribute of a <xul:menuitem> element and loading the image manually.  This code doesn't end up running through the same code path as the nsImageBoxFrame case, so we end up hitting this assertion as a result.

The least hacky way to fix this is to plug nsMenuItemIconX to the same code as nsImageBoxFrame here.  I have a patch that does this.

(Sorry for stealing this Dave, I *just* realized you had assigned this to yourself -- hope you haven't spent too much time on it yet.)

Comment 8

[(no longer active)]

Attachment: Load the menu icons for the bookmarks menu with the correct content type and principal on OSX

This patch makes nsMenuItemIconX also participate in the setup
introduced in bug 1277803.

Comment 9

[Kamil Jozwiak [:kjozwiak]]

Ehsan, when going through the same STR from comment#0 while using Ubuntu/Win, you'll get the following error message under the browser console:

[Parent 10056] WARNING: Suboptimal indexes for the SQL statement 0x653e080 (http://mzl.la/1FuID0j).: file c:/projects/mozilla-central/storage/mozStoragePrivateHelpers.cpp, line 114

Should we create a new bug or is this related to the crash that's being fixed in comment#8?

Platforms being used:

* Ubuntu 16.04.1 LTS - couldn't reproduce original crash
** fx53.0a1, buildid: 20161214214334, changeset: b1ab720c6d3e

* Win 10 Pro x64 - couldn't reproduce original crash
** fx53.0a1, buildid: 20161214214711, changeset: b1ab720c6d3e

* macOS 10.12.2 x64 - Reproduced original crash
** fx53.0a1, buildid: 20161214174132, changeset: 18b5a7a5d833

Comment 10

[(no longer active)]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #9)
> Ehsan, when going through the same STR from comment#0 while using
> Ubuntu/Win, you'll get the following error message under the browser console:
> 
> [Parent 10056] WARNING: Suboptimal indexes for the SQL statement 0x653e080
> (http://mzl.la/1FuID0j).: file
> c:/projects/mozilla-central/storage/mozStoragePrivateHelpers.cpp, line 114
> 
> Should we create a new bug or is this related to the crash that's being
> fixed in comment#8?

That's a totally different issue, unrelated to this bug.  Honestly I don't think it's even worth filing unless if you feel like figuring out where the warning is coming from...

Comment 11

[(no longer active)]

ping?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5021d272e953
Load the menu icons for the bookmarks menu with the correct content type and principal on OSX; r=baku

Comment 13

[(no longer active)]

Note that even though bug 1282124 landed for Firefox 52, this patch doesn't really need to be backported (see comment 7.)

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5021d272e953