localstore.rdf should be deleted when it is corrupt

RESOLVED FIXED

Status

defect
P3
normal
RESOLVED FIXED
18 years ago
Last year

People

(Reporter: jrgmorrison, Assigned: mossop)

Tracking

(Blocks 1 bug, {verified1.8.1.4})

Dependency tree / graph
Bug Flags:
blocking1.8.1.4 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

This bug comes out of bug 96221, and addresses one thing that should be fixed
in the parsing phase of localstore.rdf. Currently (see bug 99235) it is
possible to get illegal XML characters serialized into localstore.rdf.

When this happens, that file can no longer be parsed without throwing a
well-formedness error, and effectively that localstore.rdf can never be used
again.

The problem is that that localstore.rdf is not deleted at that time. This
breaks persistence forever in that profile (or until the user deletes the
file, but that would be a rare user who could figure out that this is what
should be done).

So, when the parser returns a fatal error to RDF during parsing of
localstore.rdf then that file should be deleted.

Giving to waterson (no, don't thank me). [But, if you point out where I might
trap the error, I may see if I can figure out how to do this deletion [side
project]].
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5
Comment on attachment 49052 [details] [diff] [review]
I threatened to... Now mock my feeble coding! (n.b. includes removal of an unused variable).

sr=waterson
Attachment #49052 - Flags: superreview+
This looks fine to me!
I was contemplating just trapping the parse error 
(NS_ERROR_HTMLPARSER_STOPPARSING I think), but the name alone 
seems like it would be a fragile thing to do. So, I guess I'll 
punt on that idea.

danm, r=?
cc'ing ccarlen, who has also hacked on localstore stuff.
Nice. This could solve much grief. Questions: What state is the datasource in
after failing to parse? Should it be reset in addition to deleting the file?
Good point. The datasource will likely contain whatever RDF/XML data could be
parsed up until this point. We probably ought to toss it, and re-create the
empty file, and re-parse the datasource from that (just to make sure that we
eventually save any subsequent changes from the currect session back).
jrgm, you wanna proceed with ccarlen's suggestions? Or ought I fess up and fix
my own crappy code?
Sorry, was away in Canada for a bit. Yep, I'll post a patch, maybe tomorrow.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
*** Bug 96221 has been marked as a duplicate of this bug. ***
I'm futuring this bug because I'm not going to get to it before mozilla-1.0.
Target Milestone: mozilla0.9.7 → Future
taking. There have been a few other cases where (for whatever reason) the 
localstore.rdf was corrupted. So, I'm gonna do this. Really. For sure. 
Assignee: waterson → jrgm
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla0.9.8
Blocks: 85026
Blocks: 88563
Target Milestone: mozilla0.9.8 → mozilla0.9.9
.
adding nsbeta1+ keyword, this needs to be fixed for MachV
Keywords: nsbeta1+
actually, this one is pretty much moot now. The datasource is now recovering 
more gracefully and overwriting a corrupt localstore.rdf when encountered 
(although this means the loss of previously persisted entries, but then, the
point of this bug was to do the same thing and delete that file when corrupt).
I just want to give this a full spin on mac and linux, but so far on win32, 
anyway that I caused the RDF to be malformed, the datasource dealt as 
gracefully as possible and not carrying over corruption to the next session.
Whiteboard: wfm
so does this still need to be plussed?
Keywords: nsbeta1+nsbeta1
.
Target Milestone: mozilla0.9.9 → mozilla1.0.1
Has this become a dup of the bug for the behavior described in commment #15? 
Anyone know which bug that was?
QA Contact: tever → nobody
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
retargeting
Target Milestone: mozilla1.0.1 → Future
i encountered a corrupt localstore.rdf twice now ...
visible effect was bookmarks gone, no search plugins and window size and position no longer saved

the first time i deleted half my user profile, now i took the time to trial-and-error it down to this one file.

i'm not even sure what exactly is stored in this file since it doesn't seem to hurt not being there at all ... still it should be detected as currupt and get deleted if that happens because "ordinary" users wouldn't know what to do then ..

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Bug 319196 is quite a large issue currently, unfortunately that bug seems to be a bit of a messy conglomerate of a number of different issues. However one of them (this is still very visible to supporters in #firefox) is that a corrupt localstore.rdf file will break basically anything that uses localstore, and it doesn't recover with a restart. Attachment 227446 [details] is an example localstore that causes this (all nulls).

The problem is that in the event of a corrupt file, the localstore never initialises, and so never even attempts to overwrite the file with clean data on shutdown. In fact every attempt to use localstore will attempt to reload the corrupt file.

This patch extends the previous to recreate a blank file and refresh the datasource when an invalid file is detected. I have tested this with the testcase and it operates correctly.

This is not actually an issue on trunk, only on 1.8 branch. On trunk the error thrown out of nsParser is overwritten with the status of the file channel at http://mxr.mozilla.org/seamonkey/source/rdf/base/src/nsRDFXMLDataSource.cpp#548 so an attempt to load a corrupt file appears to succeed (unless there is a channel error).

That said I don't see any issue with putting this on trunk as well, but it's main aim is for a branch update if possible.
Assignee: jrgmorrison → mossop.bugzilla
Attachment #49052 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #257545 - Flags: superreview?(dveditz)
Attachment #257545 - Flags: review?(axel)
QA Contact: nobody → rdf
Target Milestone: Future → ---
Flags: blocking1.8.1.4?
Not totally convinced we'll block on this, but plussing to keep on the radar for now. Patch seems straightforward, the only concern is whether Refresh might also report more transient error conditions and if so whether we should only recreate the file on some error codes and not others.
Flags: blocking1.8.1.4? → blocking1.8.1.4+
Whiteboard: wfm
Comment on attachment 257545 [details] [diff] [review]
Delete and re-create file rev 1

>Index: rdf/datasource/src/nsLocalStore.cpp
...
>+    (void)aFile->Create(nsIFile::NORMAL_FILE_TYPE, 0666);

nit, as you're doing all kinds of error checking now, pick up this error, too?

With that, r=me.
Attachment #257545 - Flags: review?(axel) → review+
just chatted with mossop (Dave) about this.  See his comment #22 about the trunk.  This sounds like something we should also do on the trunk, once we figure out the nsParser error issue.

one suggestion I have is instead of removing the corrupt localstore, moving it to the side "makeUnique(localstore.rdf.corrupt)" so that we know this happened, and we can ask advanced users to upload that file.
(In reply to comment #25)
> just chatted with mossop (Dave) about this.  See his comment #22 about the
> trunk.  This sounds like something we should also do on the trunk, once we
> figure out the nsParser error issue.

Just re-read my comment now and remembered what actually happens on trunk. The problem on branch is that since localstore sees the load fail, it never initialises properly. On trunk however since it thinks the load succeeded it initialises itself and on the next shutdown of the browser, overwrites the localstore.rdf with a fresh copy of whatever is in memory. So on trunk the corrupt file is cleaned up automatically as is. The only oddity is that a corrupt file may load some valid data (if the file stopped writing half way through for instance).
(In reply to comment #23)
> Not totally convinced we'll block on this, but plussing to keep on the radar
> for now. Patch seems straightforward, the only concern is whether Refresh might
> also report more transient error conditions and if so whether we should only
> recreate the file on some error codes and not others.

This is a fair point but it seems that it would require some more changes to overcome. From my tests with the given localstore.rdf file, the error thrown out to nsLocalStore is NS_ERROR_UNEXPECTED, generated from http://mxr.mozilla.org/mozilla1.8/source/parser/htmlparser/src/nsParser.cpp#2714. Although I cannot see any other obvious uses of this error in the code being used, I imagine that thats not an entirely safe error code to assume to mean a broken rdf file.
Posted patch patch rev 2Splinter Review
After discussion with dveditz over IRC going to stick with this as is.

This is updated with Axel's nit and a few minor style changes to match rest of file. Carrying over review just waiting on final sr.

This patch is against trunk.
Attachment #257545 - Attachment is obsolete: true
Attachment #257545 - Flags: superreview?(dveditz)
Attachment #262157 - Flags: superreview?(dveditz)
Attachment #262157 - Flags: review+
This is a matching patch that applies cleanly to the 1.8 branch.
Comment on attachment 262157 [details] [diff] [review]
patch rev 2

sr=dveditz

approved for 1.8.1.4, a=dveditz for release-drivers
Attachment #262157 - Flags: superreview?(dveditz)
Attachment #262157 - Flags: superreview+
Attachment #262157 - Flags: approval1.8.1.4+
Dave, Please land your 1.8.1.4 approved patches as soon as possible. Code freeze was pushed out to this Friday, April 27. 
Checking in mozilla/rdf/datasource/src/nsLocalStore.cpp;
/cvsroot/mozilla/rdf/datasource/src/nsLocalStore.cpp,v  <--  nsLocalStore.cpp
new revision: 1.55.24.1; previous revision: 1.55
done
Keywords: fixed1.8.1.4
Checking in mozilla/rdf/datasource/src/nsLocalStore.cpp;
/cvsroot/mozilla/rdf/datasource/src/nsLocalStore.cpp,v  <--  nsLocalStore.cpp
new revision: 1.61; previous revision: 1.60
done

Leaving open for Mossop and dveditz to figure out what they want with this bug.
This is now fixed on branch and trunk however will have no effect on trunk due to the issues mentioned in comment 22. I have filed bug 378903 to look at making a minor change to the datasource load to pass back any parse errors.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified with 2.0.0.4 rc3 on Win XP
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.