99236 - localstore.rdf should be deleted when it is corrupt

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[John Morrison]

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

Comment 1

[John Morrison]

Attachment: I threatened to... Now mock my feeble coding! (n.b. includes removal of an unused variable).

Comment 2

[Chris Waterson]

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

Comment 3

[Chris Waterson]

This looks fine to me!

Comment 4

[John Morrison]

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

Comment 5

[Chris Waterson]

cc'ing ccarlen, who has also hacked on localstore stuff.

Comment 6

[Conrad Carlen (not reading bugmail)]

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?

Comment 7

[Chris Waterson]

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

Comment 8

[Chris Waterson]

jrgm, you wanna proceed with ccarlen's suggestions? Or ought I fess up and fix
my own crappy code?

Comment 9

[John Morrison]

Sorry, was away in Canada for a bit. Yep, I'll post a patch, maybe tomorrow.

Comment 10

[Dan M]

*** Bug 96221 has been marked as a duplicate of this bug. ***

Comment 11

[Chris Waterson]

I'm futuring this bug because I'm not going to get to it before mozilla-1.0.

Comment 12

[John Morrison]

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. 

Comment 13

[John Morrison]

.

Comment 14

[Peter Trudelle]

adding nsbeta1+ keyword, this needs to be fixed for MachV

Comment 15

[John Morrison]

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.

Comment 16

[Blake Ross]

so does this still need to be plussed?

Comment 17

[John Morrison]

.

Comment 18

[Bill McGonigle (not currently reading bugmail; please contact directly)]

Has this become a dup of the bug for the behavior described in commment #15? 
Anyone know which bug that was?

Comment 19

[Samir Gehani]

adt: nsbeta1-

Comment 20

[Vedran Miletic]

retargeting

Comment 21

[Florian Schnabel]

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

Comment 22

[Dave Townsend [:mossop]]

Attachment: Delete and re-create file rev 1

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.

Comment 23

[Daniel Veditz [:dveditz]]

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.

Comment 24

[Axel Hecht]

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.

Comment 25

[(not reading, please use [e-mail] instead)]

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.

Comment 26

[Dave Townsend [:mossop]]

(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).

Comment 27

[Dave Townsend [:mossop]]

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

Comment 28

[Dave Townsend [:mossop]]

Attachment: patch rev 2

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.

Comment 29

[Dave Townsend [:mossop]]

Attachment: patch rev 2 (branch)

This is a matching patch that applies cleanly to the 1.8 branch.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 262157 [details] [diff] [review]
patch rev 2

sr=dveditz

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 31

[juan becerra [:juanb]]

Dave, Please land your 1.8.1.4 approved patches as soon as possible. Code freeze was pushed out to this Friday, April 27. 

Comment 32

[:Gijs (he/him)]

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

Comment 33

[:Gijs (he/him)]

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.

Comment 34

[Dave Townsend [:mossop]]

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.

Comment 35

[Tracy Walker [:tracy]]

verified with 2.0.0.4 rc3 on Win XP