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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: 5.js

Comment 2

[Jesse Ruderman]

[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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Dão Gottwald [:dao]]

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)?

Comment 6

[Dão Gottwald [: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?

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei can your team double check that this doesn't happen in 45?  Thanks. 
Tracking since this is likely a regression from 46.

Comment 9

[Ada [:adalucinet]]

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

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[Dão Gottwald [:dao]]

Wait, does this affect 46 without messing with hidden prefs?

Comment 12

[Marco Bonardo [:mak]]

Based on the code it should affect 46 intermittently, it's dependent on timings that are sensible to thread scheduling and architecture.

Comment 13

[Justin Dolske [:Dolske]]

I think this should be fixed by the backout -- see bug 1219788 comment 19.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixed in bug 1219788. Jesse can you confirm it's gone from 46? Or I can ask QE if you don't have time.

Comment 15

[Mayur Patil]

[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

Comment 16

[Mark Banner (:standard8)]

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.