334675 - Notify user on async IO errors

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect, P1)

Description

[Brett Wilson]

When there is an asnycIO write error, we have already continued past that point, so it is impossible to return an error to the transaction that caused the write. We currently return errors for all future operations.

We should put a message up when this happens telling the user what the error was (if we can--some of the error codes are vague or could be many things) and telling them that they will need to restart the browser.

Comment 1

[Darin Fisher]

How about logging something to the JS console instead?  This is mainly just to help with debugging places/storage, right?

Comment 2

[Brett Wilson]

This isn't for developers, it is to give better information to the user rather than just make parts of their browser break without telling them why. For example, if their disk is full or there is a write error due to some other reason.

Comment 3

[Darin Fisher]

Ah, I see.  We have something like that for app update:
http://lxr.mozilla.org/mozilla/source/toolkit/locales/en-US/chrome/mozapps/update/updates.properties#67

I agree that it's a good idea to add something like this.  It might still make sense to log more detailed error information to the JS console.  Keep the prompt as simple as possible :)

Comment 4

[Brett Wilson]

Attachment: Initial patch, no localizable strings

Comment 5

[Darin Fisher]

Comment on attachment 222551 [details] [diff] [review]
Initial patch, no localizable strings

>+  nsCOMPtr<nsAsyncWriteErrorDisplayer> displayer(new nsAsyncWriteErrorDisplayer);

I've been using nsCOMPtr<nsIRunnable> in cases like this so that
we can share a little bit of code with other events when firefox
is statically linked :-)

Comment 6

[Brett Wilson]

Attachment: Patch

I added a filename to the file structure so we can report the file with the error to the Javascript console. Unfortunately, the mrmory management around this is kind of a disaster, but I think I got it right.

Comment 7

[Darin Fisher]

Comment on attachment 222564 [details] [diff] [review]
Patch

>Index: storage/src/mozStorageAsyncIO.cpp

>+#include "nsIProxyObjectManager.h"

not needed, right?


>+  nsCString* mFilename;

You could use placement new to avoid an extra heap allocation, and
then manually call the destructor :)


>+void DisplayAsyncWriteError();

Make this static


>+        nsCOMPtr<nsIConsoleService> consoleSvc =
>+            do_GetService("@mozilla.org/consoleservice;1", &rv);
>+        if (NS_FAILED(rv)) {
>+          NS_ERROR("Couldn't get the console service for logging file error");

nit: NS_ERROR results in an assertion.  Maybe this should just be a
NS_WARNING since its not like a programmer's assumption failed?


>+          NS_ASSERTION(NS_SUCCEEDED(rv), "Couldn't log message on async error");

nit: NS_WARN_IF_FALSE


>+    nsCOMPtr<nsIPrompt> prompt = do_CreateInstance(
>+        "@mozilla.org/network/default-prompt;1", &rv);

nit: Use NS_DEFAULTPROMPT_CONTRACTID.


>+  nsCOMPtr<nsIRunnable> displayer(new nsAsyncWriteErrorDisplayer);
>+  if (! displayer) {
>+    NS_NOTREACHED("Unable to create displayer");
>+    return;
>+  }
>+  nsresult rv = NS_DispatchToMainThread(displayer);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "Can't call main thread");

Again, warnings preferred over failed assertions.  NS_NOTREACHED
is not quite right since that code could be reached if we are 
low on memory.


r=darin

Comment 8

[Brett Wilson]

Attachment: Patch addressing darin's review

Vlad: can you SR the additions to DOM, or should I get bsmedberg?

Comment 9

[Darin Fisher]

Technically, any DOM changes should be reviewed by a DOM peer (see http://www.mozilla.org/owners.html); _however_, in this case we're just talking about a locale file...

Comment 10

[Benjamin Smedberg]

Yeah, it just lives in dom/locales because that's where all gecko stuff goes. It should still be considered part of the storage "module" for review purposes.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 222907 [details] [diff] [review]
Patch addressing darin's review


>Index: dom/locales/en-US/chrome/storage.properties
>===================================================================

>+storageWriteError=There was an error writing data to the disk. This error is sometimes caused by a full disk.\n\nPlease restart Firefox

You really want to use brandShortName here...

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 222907 [details] [diff] [review]
Patch addressing darin's review

This shouldn't say Firefox in the string, and because the string lives in dom, we can't even use brandShortName.  Change "Firefox" to "this application" and file a followup on overriding the string from app/toolkit-land.

Comment 13

[Adam Guthrie]

Brett, is this patch still good to go?

Comment 14

[Brett Wilson]

I think so, I can't remember it very well.

Comment 15

[Brett Wilson]

Fixed on trunk. Leaving open for branch checkin, which I'll do if there are no trunk regressions.

Comment 16

[Brett Wilson]

(In reply to comment #12)
> Change "Firefox" to "this application" and
> file a followup on overriding the string from app/toolkit-land.

I filed bug 345880 for this. 

Comment 17

[Brett Wilson]

Attachment: Branch patch

This has all new event triggering code because it turns out the code I used didn't work on Branch. I would like to get this new event code reviewed.

Comment 18

[Darin Fisher]

Comment on attachment 231159 [details] [diff] [review]
Branch patch

>Index: storage/src/mozStorageAsyncIO.cpp
...
>+PR_STATIC_CALLBACK(void)
>+DestroyWriteErrorDisplayerPLEvent(PLEvent* aEvent)
>+{
>+}

I think this leaks memory because it does not delete the
nsAsyncWriteErrorDisplayer object in the case where the event
is successfully handled.  I think you should remove the "delete
displayer" call from DisplayAsyncWriteError into
DestroyWriteErrorDisplayerPLEvent.  Be sure to cast aEvent to
nsAsyncWriteErrorDisplayer* before deleting it.

r=darin with that fixed

Comment 19

[Brett Wilson]

Attachment: Branch patch to check in

Comment 20

[Brett Wilson]

Fixed on branch.