Closed Bug 1319908 Opened 3 years ago Closed 3 years ago

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

Categories

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

53 Branch
x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: lina, Assigned: Ehsan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

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
Do we have a regression range?
Assignee: nobody → huseby
Priority: -- → P1
Whiteboard: [domsecurity-active]
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
This is likely the culprit, but should be confirmed:

eakhgari@mozilla.com
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
Depends on: 1282124
I was able to repro the crash on changeset c9e65e3bf84a which is the one before 31c6da3081bf, Bug 1282124.
I bisected this and came up with revision 7bcb0c1694 as the first bad commit.
Blocks: 1323262
I'm going to take a look at this since it was me who landed the offending patch.
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.)
Assignee: huseby → ehsan
Blocks: 1277803
This patch makes nsMenuItemIconX also participate in the setup
introduced in bug 1277803.
Attachment #8818651 - Flags: review?(amarchesini)
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
Has STR: --- → yes
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Version: unspecified → 53 Branch
(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...
ping?
Flags: needinfo?(amarchesini)
Attachment #8818651 - Flags: review?(amarchesini) → review+
Flags: needinfo?(amarchesini)
Pushed by eakhgari@mozilla.com:
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
Note that even though bug 1282124 landed for Firefox 52, this patch doesn't really need to be backported (see comment 7.)
https://hg.mozilla.org/mozilla-central/rev/5021d272e953
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.