414715 - Notify the user if places.sqlite is locked and bookmarks and history will not work

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

STR:

1. open places.sqlite using SQLite Database Browser (or other tool)
2. make some changes to the db
3. while tool is still open, start up Firefox

Expected: not sure yet

Actual: bookmarks and history are gone and nothing really works

Comment 1

[Dietrich Ayala (:dietrich)]

We could attempt to work around locks by external apps by doing something like this:

1. detect locking error when attempting to open/create the database
2. use a different filename instead
3. have the filename in a pref, so we persistently use the alternate file

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As we can see on bug 452469 it is a really bad user experience. There is no error message at all. Users loose all of their bookmarks if an application in background has access to the places.sqlite file. That's why we should raise the severity.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Does the SQLite Database Browser locks the sqlite files on its own? The Firefox side will be solved by the patch on bug 456910.

Comment 4

[Dietrich Ayala (:dietrich)]

This needs to block 3.1. Adding uiwanted for some UX input.

Once we implement comment #1, we could restore the most recent backup, and all the user would notice is a longer startup time (and file detritus in their profile directory). or maybe we could copy the locked file.

But if for some reason we cannot get at any of the user's original data, we'd need to create a new empty db, re-import the default bookmarks set, and notify the user.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: warning dialog

here's a low-bar approach that notifies the user that the file is locked, and provides a link for more information. worth considering if we don't get a better solution in place.

Comment 6

[Dietrich Ayala (:dietrich)]

a problem with the approach in comment #1 is that the locking can continue until the offending locking app lets go, which means we would just be copying and recreating new sqlite files at every startup, without the user ever knowing there's a problem.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

David, would be the following url the right one to forward users for a solution on this problem? Or do we have a better page?

http://support.mozilla.com/tiki-view_forum_thread.php?comments_parentId=183742&forumId=3

Comment 8

[Dietrich Ayala (:dietrich)]

yeah i just put that in as a placeholder. we'd need a page dedicated to the locking issue.

Comment 9

[Tracy Walker [:tracy]]

SuMo has brief instructions to fix it here: http://support.mozilla.com/en-US/kb/Bookmarks+not+saved#Bookmarks_file_is_write_protected

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 347926 [details] [diff] [review]
warning dialog

after talking w/ sdwilsh, it's sounding like there's no feasible solution to the lock scenario except to notify the user. we have no control over the external application, and in the case of norton 360 for example, any new db file we create would also get locked.

asking for review for the code changes and ui-r. i'll work with SUMO to get a proper URL for the "more info" button.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: screenshot

probably shouldn't mention the file name in the string. any suggestions for an informative yet not overly-technical string?

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 347926 [details] [diff] [review]
warning dialog

I would reword the error text to:

"The bookmarks and history system will not be functional because one of Firefox's files has been locked by another application.  Some security software can cause this problem."

I think we should lead with the effect to the user as opposed to the cause, and the specific file name doesn't need to be mentioned until the user has reached a support document by clicking "Learn More."  This change assumes that we aren't able to accidentally lock the file ourselves.

ui-r+ with these (or similar) changes

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

Oh, and perhaps the action button should be "learn more" with a cancel button.  Odds are the user is going to want to get this solved.

This is also the appropriate time to break out the rarely used error dialog icons:

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/global/icons/error-large.png

Comment 14

[Marco Bonardo [:mak]]

"has been locked by another application": some software says "is in use by another application" i don't know if the user understand what is a lock, feels like some protection system is locking it because it's unsecure...

Comment 15

[Marco Bonardo [:mak]]

Why "Learn More" instead of the more common "Tell me more..."?

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 347926 [details] [diff] [review]
warning dialog

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -1093,7 +1093,11 @@
   gBrowser.browsers[0].removeAttribute("disablehistory");
 
   // enable global history
-  gBrowser.docShell.QueryInterface(Components.interfaces.nsIDocShellHistory).useGlobalHistory = true;
+  try {
+    gBrowser.docShell.QueryInterface(Components.interfaces.nsIDocShellHistory).useGlobalHistory = true;
+  } catch(ex) {
+    Components.utils.reportError("Places file may be locked: " + ex);
+  }
 
i guess "Places database may be locked " would be less generic than "Places file"

diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -245,6 +245,16 @@
       ww.openWindow(null, EMURL, "_blank", EMFEATURES, args);
       this._prefs.clearUserPref(PREF_EM_NEW_ADDONS_LIST);
     }
+
+    // Load the "more info" page for a locked places.sqlite
+    if (this._showPlacesLockedPage) {
+      var strings = Cc["@mozilla.org/intl/stringbundle;1"].
+                    getService(Ci.nsIStringBundleService).
+                    createBundle("chrome://browser/locale/places/places.properties");

bundleService is used in many other places in nsBrowserGlue, maybe we could make it a getter like has been done with prefs

@@ -621,6 +636,31 @@
 
+    var promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"].
+                        getService(Ci.nsIPromptService);
+    var flags = promptService.BUTTON_POS_0 * promptService.BUTTON_TITLE_OK +
+                promptService.BUTTON_POS_1 * promptService.BUTTON_TITLE_IS_STRING;
+
+    var buttonChoice = promptService.confirmEx(null, title, text, flags,
+                                               null, buttonText, null, null, {value:false});
+
+    // Show "more info" page once the browser has loaded
+    if (buttonChoice == 1)
+      this._showPlacesLockedPage = true;
   },
 
The only issue i can think of here is that buttonChoice will be 1 even if the user choose to close the dialog with X (bug 345067) so maybe you could use button 2 for "Learn More"
or as Alex said use button 0 for Learn More and button 1 for Cancel

diff --git a/browser/locales/en-US/chrome/browser/places/places.properties b/browser/locales/en-US/chrome/browser/places/places.properties
--- a/browser/locales/en-US/chrome/browser/places/places.properties
+++ b/browser/locales/en-US/chrome/browser/places/places.properties
@@ -118,3 +118,8 @@
 # for url bar autocomplete results of type "bookmark"
 # See createResultLabel() in urlbarBindings.xml 
 bookmarkResultLabel=Bookmark
+
+lockPromptTitle=Browser Startup Error
+lockPromptText=The places.sqlite file is locked. The bookmarks and history system will not be functional. Some security software can cause this problem.
+lockPromptInfoButtonText=Learn More

Do we need ellipsis here?
Plus, my comments above (would like to have an opinion from Alex)

diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
--- a/toolkit/components/places/src/nsNavHistory.cpp
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -626,8 +626,11 @@
     NS_ENSURE_SUCCESS(rv, rv);
     rv = mDBService->OpenUnsharedDatabase(mDBFile, getter_AddRefs(mDBConn));
   }
-  NS_ENSURE_SUCCESS(rv, rv);
-
+  else if (rv == NS_ERROR_FILE_IS_LOCKED) {
+    mDatabaseStatus = DATABASE_STATUS_LOCKED;
+  }
+  NS_ENSURE_SUCCESS(rv, rv);
+  

nit: trailing space

I agree to not do anything when db is locked, marking as corrupt or moving to a new db would be worst

r- is only for the problem with user clicking X and finishing on "learn more" page, apart that i think this is ok.

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #13)
> Oh, and perhaps the action button should be "learn more" with a cancel button. 
> Odds are the user is going to want to get this solved.

done

> This is also the appropriate time to break out the rarely used error dialog
> icons:
> 
> http://mxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/global/icons/error-large.png

we can't control the icon shown in nsIPromptService UI afaict. i'll file a followup to look into ways of doing this.

(In reply to comment #14)
> "has been locked by another application": some software says "is in use by
> another application" i don't know if the user understand what is a lock, feels
> like some protection system is locking it because it's unsecure...

agreed. alex?

(In reply to comment #15)
> Why "Learn More" instead of the more common "Tell me more..."?

i think "learn more" sounds a bit more optimistic, whereas "tell me" sounds slightly fatalistic.

Comment 18

[Dietrich Ayala (:dietrich)]

(In reply to comment #16)

> +lockPromptTitle=Browser Startup Error
> +lockPromptText=The places.sqlite file is locked. The bookmarks and history
> system will not be functional. Some security software can cause this problem.
> +lockPromptInfoButtonText=Learn More
> 
> Do we need ellipsis here?

ellipsis where?

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: screenshot v2

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 348713 [details] [diff] [review]
v2

>-  _idleService: null,
>-  get idleService() {
>-    if (!this._idleService)
>-      this._idleService = Cc["@mozilla.org/widget/idleservice;1"].
>-                          getService(Ci.nsIIdleService);
>+  get _idleService() {
>+    var idleSvc = Cc["@mozilla.org/widget/idleservice;1"].
>+                    getService(Ci.nsIIdleService);
>+    this.__defineGetter__("_idleService", function() idleSvc);
>     return this._idleService;
>   },

mh, i'd move all getters together, in a common code point

Effectively there's no easy way to change the icon on the prompt, it should be something to add to flags, but the interface is frozen (though adding an icon flag should not cause any incompatibilities)

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #18)
> (In reply to comment #16)
> > +lockPromptInfoButtonText=Learn More
> > 
> > Do we need ellipsis here?
> 
> ellipsis where?

for "Learn more..."

Comment 23

[Marco Bonardo [:mak]]

morphing title to be more consistent with actual implementation

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

>for "Learn more..."

This got debated at length in #ux, with options including the ellipsis, no ellipsis and the (rarely used) preceding ellipsis:

"...fantastic"

We ultimately decided that the ellipsis doesn't really make sense, because an ellipsis means "no action yet."  In this case the default action button is Learn More, so you can't really have a default action that isn't an action yet.  Of course clicking the button doesn't actually cause the user to "learn more" but it loads the page, which is close enough to the action being completed. 

Totally agree with "learn" instead of "tell me" and "in use" vs. "locked."

Comment 25

[Dietrich Ayala (:dietrich)]

thanks alex. ok, all we're blocking on now is the final support URL. i'll ping djst.

Comment 26

[David Tenser [:djst]]

So sorry about being a blocker... This is a great fix!!

So, this will be an in-product link to support, which means it should be treated just like our other in-product help links, which is (I think) a special function call with a parameter for the article. For example, we have "prefs-main" for the link to support from the Main panel of the Options window. Something like "places-locked" might be suitable here.

We would then setup appropriate .htaccess rules on support.mozilla.com to ensure this actually directs to the appropriate place (currently http://support.mozilla.com/%LOCALE%/kb/Bookmarks+not+saved#Bookmarks_file_is_write_protected)

mconnor, can you advice on parameter name?

Comment 27

[Marco Bonardo [:mak]]

one thing i forgot in the review, in the import code in browserGlue we do "importBookmarks = databaseStatus != DATABASE_STATUS_OK" but actually locked is not a status where we should do import. So please change that to:

var importBookmarks = databaseStatus == histsvc.DATABASE_STATUS_CREATE || histsvc.DATABASE_STATUS_CORRUPT;

Comment 28

[Axel Hecht [:Pike]]

djst announced this as a help link, if so, I think we should follow the pattern we already have for those, which is using app.support.baseURL (http://support.mozilla.com/1/%APP%/%VERSION%/%OS%/%LOCALE%/), and append the topic. Sample code is at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#670. That will get lockPromptInfoURL out of l10n, as it should be.

Comment 29

[Dietrich Ayala (:dietrich)]

(In reply to comment #27)
> one thing i forgot in the review, in the import code in browserGlue we do
> "importBookmarks = databaseStatus != DATABASE_STATUS_OK" but actually locked is
> not a status where we should do import. So please change that to:
> 
> var importBookmarks = databaseStatus == histsvc.DATABASE_STATUS_CREATE ||
> histsvc.DATABASE_STATUS_CORRUPT;

we'll never get there in the case of a locked db, since we return early. but i made the change anyway for future-proofing.

Comment 30

[Dietrich Ayala (:dietrich)]

Attachment: v3

Thanks Axel. Can you review that bit of the patch? The final url using that method is:

http://support.mozilla.com/1/firefox/3.1b2pre/Darwin/en-US/places-locked/

Look ok to everyone?

Comment 31

[Axel Hecht [:Pike]]

Comment on attachment 349231 [details] [diff] [review]
v3

r=me from an l10n point of view (shame on calender, huh?)

For reference, the url to redirect will not have the trailing slash, which is as it should be, though.

Comment 32

[David Tenser [:djst]]

Filed bug 466125 to get .htaccess rules for this help link on SUMO. Just for the record, until that one is fixed, this link will 404.

Comment 33

[James May [:fowl2]]

Would a more results oriented "How can I fix it?" be better? I don't think I would really want to learn anything, but I really would like my bookmarks back!

Also, is there any non-hack way of figuring out what program is holding on the the file(s)?

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

The action button needs to be phrased to the specific action that the button is going to carry out when pressed (like save, or print), so we can't really include a question or too long of a statement on the button itself.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/2e30d8025fa4

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Could this have caused ts regression on vista tboxes?

Comment 37

[Marco Bonardo [:mak]]

    1.98 +    var importBookmarks = databaseStatus == histsvc.DATABASE_STATUS_CREATE ||
    1.99 +      histsvc.DATABASE_STATUS_CORRUPT;

mh re-chechig ths code it is setting importBookmarks to histsvc.DATABASE_STATUS_CORRUPT if the first check is false? scary, i wrote it wrongly should clearly be a double check

databaseStatus == histsvc.DATABASE_STATUS_CREATE ||
databaseStatus == histsvc.DATABASE_STATUS_CORRUPT;

that could cause us to import always, the regression could be due to that

Comment 38

[Marco Bonardo [:mak]]

backed out, while cooking up a new patch let's see if this fixes Ts.
http://hg.mozilla.org/mozilla-central/rev/68e1a51bb171
http://hg.mozilla.org/mozilla-central/rev/28a7fa014b03

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 349231 [details] [diff] [review]
v3

needs new patch.

Comment 40

[Vlado Valastiak [:wladow]]

Comment on attachment 349231 [details] [diff] [review]
v3

>+lockPromptText=The bookmarks and history system will not be functional because one of Firefox's files is in use by another application. Some security software can cause this problem.

Shouldn't you we use branding variable here instead of mentioning Firefox?

Comment 41

[Robert Kaiser]

Yes, please go through GetFormattedString() or what it's called and use the brand name here, as we probably don't want to show "Firefox" when it's "Minefield" or "Shiretoko" actually.

Comment 42

[[:Cww]]

This can (may) also caused by corrupted places.sqlite if Firefox wasn't shut down correctly, see bug 464486, so we may want to mention that in the help text.

Comment 43

[Shawn Wilsher :sdwilsh]

(In reply to comment #42)
> This can (may) also caused by corrupted places.sqlite if Firefox wasn't shut
> down correctly, see bug 464486, so we may want to mention that in the help
> text.
The file shouldn't be locked if it wasn't shutdown correctly, so that's a different issue.

Comment 44

[Chris Ilias [:cilias]]

Regarding the user-doc, is 'external apps accessing places.sqlite' the only cause of this? Does places.sqlite get marked as read-only?

Comment 45

[Marco Bonardo [:mak]]

(In reply to comment #44)
> Regarding the user-doc, is 'external apps accessing places.sqlite' the only
> cause of this? Does places.sqlite get marked as read-only?

places.sqlite is mantained locked (in use) by the external app, it's not marked  read-only.

Comment 46

[Marco Bonardo [:mak]]

while doing some test i noticed that if a third party app opens places.sqlite and calls an exclusive flock the error sqlite returns is not FILE_LOCKED but SQLITE_IOERR, this error is not served by mozStorage, so i'm going to file a bug on that for mozStorage and we should detect it as a possible locking error.

Comment 47

[Marco Bonardo [:mak]]

also, we throw when getting history service if file is locked, so we can't check for databaseStatus locked.

Comment 48

[Marco Bonardo [:mak]]

storage bug for SQLITE_IOERR is bug 467856

Comment 49

[Dietrich Ayala (:dietrich)]

Attachment: v3

w/ marco's change and fixes the l10n issue.

Comment 50

[Dietrich Ayala (:dietrich)]

(In reply to comment #47)
> also, we throw when getting history service if file is locked, so we can't
> check for databaseStatus locked.

hm, at first i though this was true. however, if i force mDatabaseStatus = locked before checking rv, histsvc.databaseStatus has that value in nsBrowserGlue.

Comment 51

[Marco Bonardo [:mak]]

(In reply to comment #50)
> (In reply to comment #47)
> > also, we throw when getting history service if file is locked, so we can't
> > check for databaseStatus locked.
> 
> hm, at first i though this was true. however, if i force mDatabaseStatus =
> locked before checking rv, histsvc.databaseStatus has that value in
> nsBrowserGlue.

but are you also making init return NS_ERROR_FILE_IS_LOCKED? since the real issue is that getService would throw, and we would never reach the if (histsvc.databaseStatus...). moreover even if we handle the exception at that point histsvc should be invalid. What am i missing?

Comment 52

[Dietrich Ayala (:dietrich)]

Attachment: v4

changes: don't throw if the file is locked, also disable history.

Comment 53

[Marco Bonardo [:mak]]

i'm still unsure about this, if we don't throw a third party implementer (and probably also part of our code) will expect that the history service is ready to go, at that point i think that even if isHistoryDisabled is true we will still try to add bookmarks, annotations and so on... i think we need to talk a bit about this on IRC.

Other changes appear goood, notice we need bug 467856 and will have to change the compare to check also || rv == NS_ERROR_STORAGE_IOERR

Comment 54

[Marco Bonardo [:mak]]

the main issue is that thinking the history service has inited correct, code could try to use the db connection and cause crashes

Comment 55

[Chris Ilias [:cilias]]

Is there a list of common apps that cause this? I would think the first piece of advice in the user-doc would be to quit those apps.

Comment 56

[Dietrich Ayala (:dietrich)]

Attachment: v5

instead always throw in init(), so extensions etc don't try to use the services, and handle those result codes in nsBrowserGlue in order to show the dialog.

Comment 57

[Dietrich Ayala (:dietrich)]

Attachment: v5.1

fixed

Comment 58

[Marco Bonardo [:mak]]

Comment on attachment 352452 [details] [diff] [review]
v5.1

>+    var histsvc = null;
>+    try {
>+      histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>                   getService(Ci.nsINavHistoryService);
>-    var databaseStatus = histsvc.databaseStatus;
>+    }
>+    catch(ex) {
>+      if (ex.result == Cr.NS_ERROR_FILE_IS_LOCKED ||
>+          ex.result == Cr.NS_ERROR_STORAGE_IOERR ||
>+          ex.result == Cr.NS_ERROR_STORAGE_BUSY) {
>+        this._showPlacesLockedAlert();
>+      }
>+      Cu.reportError("History service unable to initialize: " + ex);
>+      return;
>+    }

This won't work for a simple reason, when we throw we don't send the "places-init-complete" notification, and initPlaces is called when we receive that notification, so it's never called for a locked file.
Actually that is because nsDBFlush inits history before nsBrowserGlue (when it creates bookmarks observer), so actually i think the only valid solution is to detect those errors in the backend and fire a places-database-locked notification, the same way we do for "places-init-complete" (enqueuing an event to the main thread), in nsBrwoserGlue we could then observe the notification and fire up the dialog.

Comment 59

[Marco Bonardo [:mak]]

and notice as soon as we serve the notification we should remove the observer to avoid firing up the dialog every time something tries to init history.

Comment 60

[Dietrich Ayala (:dietrich)]

Attachment: v6

new approach. includes more fixes for ensuring the browser is usable even when Places isn't. there'l always be some breakage when the Places services don't init, but basic browsing should be expected to work.

Comment 61

[Marco Bonardo [:mak]]

Comment on attachment 352780 [details] [diff] [review]
v6

>   /**
>+   * Set when database is locked
>+   */
>+  const unsigned short DATABASE_STATUS_LOCKED = 3;
>+

i would completely remove this status and all references to it, i guess it could make third party implementers work on the false assumption they can get a valid instance of the service and read status to check for locking, while they should listen on the notification instead since initing the service will throw.

Also, while there, could make sense adding a note in the idl about the two notifications we send to the main thread so implementers are aware of them.

>@@ -430,6 +436,15 @@
> 
>   // init db file
>   rv = InitDBFile(PR_FALSE);
>+  if (rv == NS_ERROR_FILE_IS_LOCKED || rv ==  NS_ERROR_STORAGE_IOERR ||
>+      rv == NS_ERROR_STORAGE_BUSY) {

i think we will add more checks in bug 464486, ok for now but this should include read only or wrong permission databases.

>+    // If the database is locked, send out a notification and do not
>+    // continue initialization.
>+    nsCOMPtr<PlacesEvent> lockedEvent = new PlacesEvent(PLACES_DB_LOCKED_EVENT_TOPIC);
>+    rv = NS_DispatchToMainThread(lockedEvent);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    return NS_ERROR_FAILURE;

could make sense return the original rv we got from the db init

apart those code changes, we still have an issue, even if the dialog is now shown we are acting on a notification, so the browser init goes on. The prompt is open but immediately after the browser window opens covering the dialog. At this point the button "Learn More" does not work anymore because when the user clicks the button we have already passed over the if that checks for it.

I'm thinking we should move to a browser notification bar that scrolls down with a button to learn more and the classic X to close. this should probably be done setting an internal attribute like _isPlacesDatabaseLocked and checking it onBrowserStartup. the problem is that i can't see the sessionstore-windows-restored notification, i suspect something is blocking it (from the dox it sounds as a notification fired even if there's nothing to restore), if we don't get that notification onBrowserStartup won't run.

Comment 62

[Marco Bonardo [:mak]]

(In reply to comment #61)
> the problem is that i can't see the
> sessionstore-windows-restored notification, i suspect something is blocking it
> (from the dox it sounds as a notification fired even if there's nothing to
> restore), if we don't get that notification onBrowserStartup won't run.

nevermind this, i can get the notification now (was a build issue probably), i've tested the notificationBox path and that works flawlessy and is less verbose than the nsIPrompt implementation... if Faaborg is ok with that path i think it's a good way to go.

Comment 63

[Marco Bonardo [:mak]]

Attachment: v6.1

since i've implemented the notificationBox to see if that was working i attach the changed patch to avoid doing the work twice, this still needs:
- fix my review comments above
- get ui-r from ux team
- add an accesskey for the button
- decide on notification priority (i've set it to critical-medium, so it's critical because most of the browser functions are not there, medium because you can still use the browser)

Comment 64

[Marco Bonardo [:mak]]

Attachment: screenshot with notification box

Alex do you think would it be ok to show a notification box like this instead of a dialog since it could be hard to show a blocking dialog on top of browser window?

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The information which I got on another bug a while back was that notification boxes are only used for site related things, e.g. password bar, add-ons installation. But here we have a application wide issue which wont fit into this demand. But lets see what Alex think about.

Comment 66

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 352841 [details]
screenshot with notification box

This is better than the dialog from a UI perspective anyway, I really should have thought about using a notification bar.  ui-r++ :)

Comment 67

[Alex Faaborg [:faaborg] (Firefox UX)]

>The information which I got on another bug a while back was that notification
>boxes are only used for site related things, e.g. password bar, add-ons
>installation. But here we have a application wide issue which wont fit into
>this demand. But lets see what Alex think about.

Yeah, this was the case until we landed the about:rights UI.  In general I think we should try to create a notification system that makes a better distinction between browser level message and page level messages, but in the meantime these types of notifications are considerably less invasive than a modal dialog, and still convey thier message effectively.

Will this notification go away on page navigate similar to others, or does it need to be dismissed?  Ideally we should have it go away on page navigate, which allows the user to read and ignore, and is better than a forced choice dialog.

Comment 68

[Marco Bonardo [:mak]]

(In reply to comment #67)
> Will this notification go away on page navigate similar to others, or does it
> need to be dismissed?  Ideally we should have it go away on page navigate,
> which allows the user to read and ignore, and is better than a forced choice
> dialog.

the behaviour should be configurable, i've setup it as to persist until a choice is made, but can be changed like other notification boxes

Comment 70

[Dietrich Ayala (:dietrich)]

Attachment: v6.2

fixed marco comments and todos on my and his previous patches.

Comment 71

[Dietrich Ayala (:dietrich)]

Comment on attachment 353249 [details] [diff] [review]
v6.2

Hi Gavin, requesting review for the /browser changes.

Comment 72

[Marco Bonardo [:mak]]

Comment on attachment 353249 [details] [diff] [review]
v6.2

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
nsBrowserGlue__showPlacesLockedNotificationBox() {
>+    // stop observing, so further attempts to load history service
>+    // do not show the prompt.
>+    const osvr = Cc['@mozilla.org/observer-service;1'].
>+                 getService(Ci.nsIObserverService);
>+    osvr.removeObserver(this, "places-database-locked");
>+
>+    // notify the user

this comment is probably the remaining part of old code, is mostly useless, hwv this is an ignorable nit.


>     rv = observerService->NotifyObservers(nsnull,
>-                                          PLACES_INIT_COMPLETE_EVENT_TOPIC,
>+                                          mTopic,
>                                           nsnull);

is this to not pollute blame? could take 1 line actually.

>@@ -629,6 +635,14 @@
>     rv = mDBFile->Append(DB_FILENAME);
>     NS_ENSURE_SUCCESS(rv, rv);
>     rv = mDBService->OpenUnsharedDatabase(mDBFile, getter_AddRefs(mDBConn));
>+  }
>+  

nit: trailing space

i did a functionality test, and all appear correct now, i like this solution too.

r=mak77

Comment 73

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 353249 [details] [diff] [review]
v6.2

r=me, just a few nits; feel free to ignore them (apart from the comment about entity names).

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>-  PlacesStarButton.init();
>+  try {
>+    PlacesStarButton.init();
>+  } catch(ex) {
>+    Components.utils.reportError("Error initializing the star button: " + ex);
>+  }

Put this try/catch inside init() instead? Callers shouldn't have to deal with it failing, and makes it more obvious what exactly we're expecting to fail.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

> BrowserGlue.prototype = {

>+    var prefs = Cc["@mozilla.org/preferences-service;1"].
>+                getService(Ci.nsIPrefBranch);
>+    this.__defineGetter__("_prefs", function() prefs);
>+    return this._prefs;

Where'd this memoizing style come from? I prefer e.g. http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/utils.js#111 , avoiding the unnecessary getter.

>+    // Load the "more info" page for a locked places.sqlite
>+    if (this._isPlacesDatabaseLocked) {
>+      this._showPlacesLockedNotificationBox();

I guess we're sure that the notification will have been sent by now because browser window startup triggers it? Might be worth a comment, since I had forgotten that onBrowserStartup() ran from "sessionstore-windows-restored".

>+  _showPlacesLockedNotificationBox: function nsBrowserGlue__showPlacesLockedNotificationBox() {
>+    // stop observing, so further attempts to load history service
>+    // do not show the prompt.
>+    const osvr = Cc['@mozilla.org/observer-service;1'].
>+                 getService(Ci.nsIObserverService);
>+    osvr.removeObserver(this, "places-database-locked");

Conceptually I would prefer if this code was in observe() method along with the call to showPlacesLockedNotificationBox, to make it more obvious what's going on.

>diff --git a/browser/locales/en-US/chrome/browser/places/places.properties b/browser/locales/en-US/chrome

>+# LOCALIZATION NOTE (lockPromptText)
>+# %S will be replaced with the application name.
>+lockPromptTitle=Browser Startup Error
>+lockPromptText=The bookmarks and history system will not be functional because one of %S's files is in use by another application. Some security software can cause this problem.
>+lockPromptInfoButtonText=Learn More
>+lockPromptInfoButtonAccessKey=L

I think lockPrompt.title, lockPrompt.text, lockPromptInfoButton.label and lockPromptInfoButton.accesskey would work better as entity names (some l10n tools indicate the relationship using the names).

Comment 74

[Chris Ilias [:cilias]]

For clarification (for user documentation), will this message come up if places.sqlite is corrupt, instead of locked? Or is it just if places.sqlite is locked?

Comment 75

[Marco Bonardo [:mak]]

(In reply to comment #74)
> For clarification (for user documentation), will this message come up if
> places.sqlite is corrupt, instead of locked? Or is it just if places.sqlite is
> locked?

If the file can't be open, if it can be open but is corrupt we will replace it and restore from JSON.

Comment 76

[Chris Ilias [:cilias]]

So if it cannot be open (due to being locked or corrupt), this message will come up?

Comment 77

[Dietrich Ayala (:dietrich)]

(In reply to comment #76)
> So if it cannot be open (due to being locked or corrupt), this message will
> come up?

No. If the database cannot be opened because:

Corrupt: Recreate the database, import the most recent bookmarks backup.

Other: Do not recreate the database, show the notification.

Comment 78

[Marco Bonardo [:mak]]

Attachment: test for notifications

this is a test for Places notifications we can add to this patch, it tests for locked and complete notifications.

Comment 79

[Dietrich Ayala (:dietrich)]

Attachment: v6.3

includes fixes for all comments + marco's test

Comment 80

[Marco Bonardo [:mak]]

Comment on attachment 353363 [details] [diff] [review]
v6.3

>@@ -130,9 +157,18 @@
>         break;
>       case "places-init-complete":
>         this._initPlaces();
>+        this._observerService.removeObserver(this, "places-init-complete");
>+        // no longer needed, since history was initialized completely.
>+        this._observerService.removeObserver(this, "places-database-locked");

what's the point of removing the observer both here and in _dispose?
If we  remove it here there's no need to remove it later, i think Gavin's comment was referring only to the locked case below. Otherwise there's no need to remove it also in _dispose.

>@@ -629,6 +633,14 @@
>     rv = mDBFile->Append(DB_FILENAME);
>     NS_ENSURE_SUCCESS(rv, rv);
>     rv = mDBService->OpenUnsharedDatabase(mDBFile, getter_AddRefs(mDBConn));
>+  }
>+ 

nit: Probably still a trailing space

>+ * Portions created by the Initial Developer are Copyright (C) 2007

please update the year in the test header, i forgot to do that

Comment 81

[Dietrich Ayala (:dietrich)]

(In reply to comment #73)
> >+    var prefs = Cc["@mozilla.org/preferences-service;1"].
> >+                getService(Ci.nsIPrefBranch);
> >+    this.__defineGetter__("_prefs", function() prefs);
> >+    return this._prefs;
> 
> Where'd this memoizing style come from? I prefer e.g.
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/utils.js#111
> , avoiding the unnecessary getter.

that style of memoization doesn't work if the getter is in a prototype.

(In reply to comment #80)
> what's the point of removing the observer both here and in _dispose?
> If we  remove it here there's no need to remove it later, i think Gavin's
> comment was referring only to the locked case below. Otherwise there's no need
> to remove it also in _dispose.

i had locally removed both topics in _dispose, since they're never sent a second time, but forgot to qrefresh before uploading the patch. same goes for the other nits.

Comment 82

[Dietrich Ayala (:dietrich)]

(In reply to comment #81)
> i had locally removed both topics in _dispose, since they're never sent a
> second time, but forgot to qrefresh before uploading the patch. same goes for
> the other nits.

clarification: the notifications only need to be received *once* by nsBrowserGlue. the locked notification will be sent out every time the history service attempts to initialize.

Comment 83

[Dietrich Ayala (:dietrich)]

Attachment: v6.4

nits fixed

Comment 84

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 353510 [details] [diff] [review]
v6.4

>diff --git a/browser/locales/en-US/chrome/browser/places/places.properties b/browser/locales/en-US/chrome/browser/places/places.properties

>+lockPrompt.title=Browser Startup Error
>+lockPrompt.text=The bookmarks and history system will not be functional because one of %S's files is in use by another application. Some security software can cause this problem.
>+lockPromptInfoButton.text=Learn More
>+lockPromptInfoButton.accessKey=L

One last nit: ".label" for button labels, ".text" for dialog text, so lockPromptInfoButton.text ->lockPromptInfoButton.label.

Comment 85

[Dietrich Ayala (:dietrich)]

checked in on trunk: http://hg.mozilla.org/mozilla-central/rev/1a6e3ece12c3

Comment 86

[Dietrich Ayala (:dietrich)]

(In reply to comment #84)
> One last nit: ".label" for button labels, ".text" for dialog text, so
> lockPromptInfoButton.text ->lockPromptInfoButton.label.

fixed

Comment 87

[Dietrich Ayala (:dietrich)]

backed out due to stringbundle leaks.

Comment 91

[Chris Ilias [:cilias]]

user-doc-complete
http://support.mozilla.com/kb/The+bookmarks+and+history+system+will+not+be+functional

Comment 92

[Dietrich Ayala (:dietrich)]

log of leakage: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229637145.1229640033.17901.gz

Comment 93

[Dietrich Ayala (:dietrich)]

checked in w/ leak fix (moving smart getters into constructor):

http://hg.mozilla.org/mozilla-central/rev/e544c324fcb5

Comment 94

[Dietrich Ayala (:dietrich)]

and backed out again, still leaks:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1230593893.1230598222.11844.gz&fulltext=1

Comment 95

[Marco Bonardo [:mak]]

i would try changing:
   3.270 +    var strings = this._placesBundle;
   3.271  
   3.272      var callback = {
   3.273 -      _placesBundle: Cc["@mozilla.org/intl/stringbundle;1"].
   3.274 -                     getService(Ci.nsIStringBundleService).
   3.275 -                     createBundle("chrome://browser/locale/places/places.properties"),
   3.276 +      _placesBundle: strings,

Comment 96

[Marco Bonardo [:mak]]

Attachment: v6.5

this locally fixes the additional stringBundleService leak (and related ones) for me, so i'm going to try pushing it, includes previous getters fixes, plus the removal of placesBundle getter.

Comment 97

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/472b245a8b2e
waiting for results before resolving.

Comment 98

[Marco Bonardo [:mak]]

the only difference i still see in leak logs before/after this push is:
TEST-PASS | runtests-leaks | leaked 85 instances of XPCWrappedNative with size 48 bytes each (4080 bytes total)
on OS X 10.5.2 mozilla-central unit test
previously was 84 instances, apart that, string bundle service leak is fixed, and we are fairly below any leak limit. Other leaks are tracked in bug 462545, so, since we are really near the string freeze, i would suggest to leave the patch in and provide a followup fix for the remaining leak, or fix it with bug 462545.

Comment 99

[Marco Bonardo [:mak]]

i mean bug 465952, not bug 462545

Comment 100

[Marco Bonardo [:mak]]

locally disabling browser_sanitize-timespans.js test solves all leaks, so fixing the leak in bug 465952 sounds correct, i'm marking FIXED, should land on 1.9.1 before the string freeze.

Comment 101

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d666a12db06d

Comment 102

[Marco Bonardo [:mak]]

Attachment: lock php

a script to lock the db for the QA

Comment 103

[Dietrich Ayala (:dietrich)]

to lock the db on the command line:

> perl -e 'open(outf, ">>places.sqlite"); flock(outf, 2); sleep();'

that'll keep it locked until you ctl+c to kill it.

Comment 104

[Samuel Sidler (old account; do not CC)]

Dietrich: Is there any way to port this patch to 1.9.0 without any l10n changes? What would be required and how safe would such a change be?

Comment 105

[Dietrich Ayala (:dietrich)]

Hm.

l10n: There are a lot of particular strings there. We might be able to piecemeal create something usable from existing ones. Maybe there's a l10n peep who knows the available strings well enough to do this?

work: The patch depends on some other 1.9.1 changes to how Places is initialized. Probably a day or two to get it working without them, but is possible.

safety: This code has been in nightlies for a while, but hasn't gone out in a beta yet, so we don't have very widespread coverage. That said, there haven't been problems reported yet.

Comment 107

[Mike Connor [:mconnor]]

I don't think we want to mess around with this for 3.0, 3.1 will be out soon enough.

Comment 108

[Samuel Sidler (old account; do not CC)]

Sounds find to me.

Comment 109

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090203 Shiretoko/3.1b3pre

Comment 115

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv