Closed Bug 414715 Opened 16 years ago Closed 16 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b3

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

(Keywords: late-l10n, user-doc-complete, verified1.9.1)

Attachments

(4 files, 14 obsolete files)

52.89 KB, image/png
Details
54.84 KB, image/PNG
faaborg
: ui-review+
Details
24.50 KB, patch
Details | Diff | Splinter Review
99 bytes, text/plain
Details
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
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
Blocks: 452469
Flags: blocking-firefox3.1?
Target Milestone: --- → Firefox 3.1
Priority: -- → P1
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.
Severity: normal → critical
Does the SQLite Database Browser locks the sqlite files on its own? The Firefox side will be solved by the patch on bug 456910.
Depends on: 456910
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.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Keywords: uiwanted
Assignee: nobody → dietrich
Attached patch warning dialog (obsolete) — Splinter Review
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.
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.
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
yeah i just put that in as a placeholder. we'd need a page dedicated to the locking issue.
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.
Attachment #347926 - Flags: ui-review?(faaborg+bugzilla)
Attachment #347926 - Flags: review?(mak77)
Attached image screenshot (obsolete) —
probably shouldn't mention the file name in the string. any suggestions for an informative yet not overly-technical string?
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
Attachment #347926 - Flags: ui-review?(faaborg+bugzilla) → ui-review+
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
"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...
Why "Learn More" instead of the more common "Tell me more..."?
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.
Attachment #347926 - Flags: review?(mak77) → review-
(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.
(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?
Attached patch v2 (obsolete) — Splinter Review
Attachment #347926 - Attachment is obsolete: true
Attachment #348713 - Flags: review?(mak77)
Attached image screenshot v2
Attachment #348063 - Attachment is obsolete: true
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)
Attachment #348713 - Flags: review?(mak77) → review+
(In reply to comment #18)
> (In reply to comment #16)
> > +lockPromptInfoButtonText=Learn More
> > 
> > Do we need ellipsis here?
> 
> ellipsis where?

for "Learn more..."
morphing title to be more consistent with actual implementation
Summary: bookmarks and history broken if places.sqlite is locked → Notify the user if places.sqlite is locked and bookmarks and history will not work
>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."
thanks alex. ok, all we're blocking on now is the final support URL. i'll ping 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?
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;
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.
(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.
Attached patch v3 (obsolete) — Splinter Review
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?
Attachment #348713 - Attachment is obsolete: true
Attachment #349231 - Flags: review?
Attachment #349231 - Flags: review? → review?(l10n)
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.
Attachment #349231 - Flags: review?(l10n) → review+
Keywords: uiwanted
Whiteboard: [has patch][has reviews]
Blocks: 466125
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.
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)?
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.
Blocks: 464486
Pushed http://hg.mozilla.org/mozilla-central/rev/2e30d8025fa4
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Could this have caused ts regression on vista tboxes?
    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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 349231 [details] [diff] [review]
v3

needs new patch.
Attachment #349231 - Attachment is obsolete: true
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?
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.
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.
(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.
Whiteboard: [has patch][has reviews] → [needs new patch]
Regarding the user-doc, is 'external apps accessing places.sqlite' the only cause of this? Does places.sqlite get marked as read-only?
(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.
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.
also, we throw when getting history service if file is locked, so we can't check for databaseStatus locked.
storage bug for SQLITE_IOERR is bug 467856
Depends on: 467856
Attached patch v3 (obsolete) — Splinter Review
w/ marco's change and fixes the l10n issue.
Attachment #351436 - Flags: review?(mak77)
(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.
Whiteboard: [needs new patch] → [needs review marco]
(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?
Whiteboard: [needs review marco] → [needs new patch]
Attached patch v4 (obsolete) — Splinter Review
changes: don't throw if the file is locked, also disable history.
Attachment #351436 - Attachment is obsolete: true
Attachment #351990 - Flags: review?(mak77)
Attachment #351436 - Flags: review?(mak77)
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
Whiteboard: [needs new patch] → [needs all dependant bugs]
the main issue is that thinking the history service has inited correct, code could try to use the db connection and cause crashes
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.
Attached patch v5 (obsolete) — Splinter Review
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.
Attachment #351990 - Attachment is obsolete: true
Attachment #352451 - Flags: review?(mak77)
Attachment #351990 - Flags: review?(mak77)
Attached patch v5.1 (obsolete) — Splinter Review
fixed
Attachment #352451 - Attachment is obsolete: true
Attachment #352452 - Flags: review?(mak77)
Attachment #352451 - Flags: review?(mak77)
Whiteboard: [needs all dependant bugs] → [has wip patch][needs review marco]
Blocks: 469070
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.
Attachment #352452 - Flags: review?(mak77) → review-
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.
Blocks: 442421
Whiteboard: [has wip patch][needs review marco] → [needs new patch]
Attached patch v6 (obsolete) — Splinter Review
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.
Attachment #352452 - Attachment is obsolete: true
Attachment #352780 - Flags: review?(mak77)
Whiteboard: [needs new patch] → [needs review marco
Whiteboard: [needs review marco → [needs review marco]
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.
Attachment #352780 - Flags: review?(mak77)
(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.
Attached patch v6.1 (obsolete) — Splinter Review
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)
Attachment #352840 - Attachment is patch: true
Attachment #352840 - Attachment mime type: application/octet-stream → text/plain
Whiteboard: [needs review marco] → [needs new patch]
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?
Attachment #352841 - Flags: ui-review?(faaborg)
Attachment #352841 - Attachment is patch: false
Attachment #352841 - Attachment mime type: text/plain → image/PNG
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.
Status: REOPENED → ASSIGNED
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++ :)
Attachment #352841 - Flags: ui-review?(faaborg) → ui-review+
>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.
(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
Attached patch v6.2 (obsolete) — Splinter Review
fixed marco comments and todos on my and his previous patches.
Attachment #352780 - Attachment is obsolete: true
Attachment #352840 - Attachment is obsolete: true
Attachment #353249 - Flags: review?(mak77)
Comment on attachment 353249 [details] [diff] [review]
v6.2

Hi Gavin, requesting review for the /browser changes.
Attachment #353249 - Flags: review?(gavin.sharp)
Whiteboard: [needs new patch] → [needs review marco and gavin]
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
Attachment #353249 - Flags: review?(mak77) → review+
Whiteboard: [needs review marco and gavin] → [needs review on browser changes gavin]
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).
Attachment #353249 - Flags: review?(gavin.sharp) → review?(mak77)
Attachment #353249 - Flags: review?(mak77) → review+
Whiteboard: [needs review on browser changes gavin] → [has reviews][needs new patch]
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?
(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.
So if it cannot be open (due to being locked or corrupt), this message will come up?
(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.
Attached patch test for notifications (obsolete) — Splinter Review
this is a test for Places notifications we can add to this patch, it tests for locked and complete notifications.
Flags: in-testsuite?
Attached patch v6.3 (obsolete) — Splinter Review
includes fixes for all comments + marco's test
Attachment #353249 - Attachment is obsolete: true
Attachment #353327 - Attachment is obsolete: true
Attachment #353363 - Flags: review?(mak77)
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
Attachment #353363 - Flags: review?(mak77) → review+
Blocks: 469970
No longer blocks: 469970
(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.
(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.
Attached patch v6.4 (obsolete) — Splinter Review
nits fixed
Attachment #353363 - Attachment is obsolete: true
Whiteboard: [has reviews][needs new patch] → [has reviews]
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.
checked in on trunk: http://hg.mozilla.org/mozilla-central/rev/1a6e3ece12c3
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
(In reply to comment #84)
> One last nit: ".label" for button labels, ".text" for dialog text, so
> lockPromptInfoButton.text ->lockPromptInfoButton.label.

fixed
Target Milestone: Firefox 3.1 → Firefox 3.1b3
backed out due to stringbundle leaks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [has reviews] → [needs new patch due to stringbundle leaking]
checked in w/ leak fix (moving smart getters into constructor):

http://hg.mozilla.org/mozilla-central/rev/e544c324fcb5
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,
Attached patch v6.5Splinter Review
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.
Attachment #353510 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/472b245a8b2e
waiting for results before resolving.
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.
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.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Whiteboard: [needs new patch due to stringbundle leaking]
Whiteboard: [baking on trunk]
Depends on: 471788
Attached file lock php
a script to lock the db for the QA
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.
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?
Flags: wanted1.9.0.x?
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.
I don't think we want to mess around with this for 3.0, 3.1 will be out soon enough.
Sounds find to me.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090203 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: