Bookmarks toolbar appears in new profile with javascript.options.parallel_parsing=false

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
P1
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

({regression})

Trunk
regression
Points:
---

Firefox Tracking Flags

(firefox45 unaffected, firefox46+ fixed, firefox47 fixed)

Details

Attachments

(1 attachment)

214 bytes, text/plain
Details
(Reporter)

Description

2 years ago
Steps:

mkdir -p ~/tempprof/
cp 5.js ~/tempprof/prefs.js
firefox -no-remote -profile ~/tempprof/
rm -rf ~/tempprof/

Result: Browser window appears, then the bookmarks toolbar slides in.

Expected: The bookmarks toolbar should not be shown.

I'm guessing this is a regression from bug 1219788.
Flags: needinfo?(dao)
(Reporter)

Comment 1

2 years ago
Created attachment 8711891 [details]
5.js
(Reporter)

Comment 2

2 years ago
[Tracking Requested - why for this release]:

Probably a regression from bug 1219788, and I'm not sure that setting this obscure pref is the only way to trigger it.
tracking-firefox46: --- → ?
ugh, do you see bookmarks on the toolbar?
I suspect using bookmark notifications is a little bit too broad, we still create a smart bookmark on the toolbar and also default bookmarks could be created there.

I wonder if we should just handle UI actions (after a call to showBookmarksDialog, migration or the star button) and don't handle bookmarks created by code or add-ons... That complicates the code quite a bit though.
I'd probably suggest to backout bug 1219788 if we don't get a solution soon, or we may break the wins of having an hidden toolbar by default on new profiles.

If we just want to skip over default bookmarks we could start listening for bookmarks changes after places-browser-init-complete. But this wouldn't change the fact add-ons or Sync could add a bookmark to the toolbar and make it visible, and I don't know if we want that.
I don't understand yet what exactly this bug is about. Jesse says he only flipped some obscure JS prefs and Marco seems to suspect we're handling a bookmark notification that we should rather ignore. It's not clear to me how bring these two theories together.

Jesse, to reiterate Marco's question, do you see bookmarks on the toolbar?

Marco, any idea where that "wrong" notification might be coming from in a new profile (i.e. no add-ons, no Sync)?
Flags: needinfo?(mak77)
Flags: needinfo?(jruderman)

Updated

2 years ago
Flags: needinfo?(dao)
(In reply to Jesse Ruderman from comment #1)
> Created attachment 8711891 [details]
> 5.js

When trying to reproduce this, I removed the first three entries in that file, leaving only user_pref("javascript.options.parallel_parsing", false) and this still triggered the bug. The only bookmarks present on the toolbar were "Most Visited" and "Getting Started".

So what exactly does javascript.options.parallel_parsing do? Is this experimental, could it break how we expect scripts to run? Or could this mean there's a race condition in our scripts?

Updated

2 years ago
Summary: Bookmarks toolbar appears spontaneously, depending on JS optimizations → Bookmarks toolbar appears in new profile with javascript.options.parallel_parsing=false
I suspect it's a timing thing, could be our observer is added (in delayed startup) before or after default bookmarks are handled in browserGlue.
Flags: needinfo?(mak77)

Updated

2 years ago
Flags: needinfo?(jruderman)
Andrei can your team double check that this doesn't happen in 45?  Thanks. 
Tracking since this is likely a regression from 46.
status-firefox45: --- → unaffected
status-firefox46: --- → affected
tracking-firefox46: ? → +
Flags: needinfo?(andrei.vaida)

Comment 9

2 years ago
This issue is *not* reproducible on 45 beta 2 (Build ID: 20160201143558), under Ubuntu 14.04 32-bit.

Also, confirming the regression range:
Last good Nightly: 2016-01-06 
First bad Nightly: 2016-01-07
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d6ffc7a08b6&tochange=e0bcd16e1d4b
Flags: needinfo?(andrei.vaida)
I only see 2 solutions here:
1. remove the current patch, try to handle this from the UI code. That means only when the user adds a bookmark through the star panel. It won't catch all the cases, but would catch the ones we care most (main menu, context menu star, star button). It would have a lower performance cost. We could try using the popuphidden to figure if a bookmark was added to the toolbar.
2. Start listening later, at places-browser-init-complete. Catches everything but it's more expensive cause it stays listening all the time (like the current code). Could be listening too much, so on a background Sync or an add-on adding a bookmark, the toolbar could appear abruptly.

I'd probably vote for backing out the current patch and try path 1, my only doubt is whether it's easy to get the parentGuid in popuphidden, but we have _itemId and we can ask to gEditItemOverlay, so it should be feasible.

Updated

2 years ago
Priority: -- → P1
Wait, does this affect 46 without messing with hidden prefs?
Based on the code it should affect 46 intermittently, it's dependent on timings that are sensible to thread scheduling and architecture.
I think this should be fixed by the backout -- see bug 1219788 comment 19.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Fixed in bug 1219788. Jesse can you confirm it's gone from 46? Or I can ask QE if you don't have time.
status-firefox46: affected → fixed
status-firefox47: affected → fixed
Flags: needinfo?(jruderman)

Comment 15

2 years ago
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
This is a 2 year-old NI for FF 46 for request for verification. AFAIK there's been no other reports, so I think we can assume it has gone.
Flags: needinfo?(jruderman)
You need to log in before you can comment on or make changes to this bug.