82590 - mork assertions in history code when clicking on certain urls

Status: VERIFIED WORKSFORME

(MailNews Core :: Database, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

The subject pretty much sez it all, I think.  Filesystem corruption is a fact of
life, and Mozilla should be more robust in the face of it.

Comment 1

[David :Bienvenu]

AFAIK, we're plenty robust in the fact of corrupt .msf files - we don't crash,
we delete them and regenerate the data. Aren't you really complaining about the
annoying asserts when running a debug build? I suppose I could turn off the
asserts, but for the record, I never see these asserts, and my db's never get
corrupted. Are you running two clients against the same profile?

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I can consistently get a corrupted database using a testcase to bug 73958.

The testcase for that bug consists of a XUL file that I open as if it was normal
file. The XUL file includes a JS file. In onload handler we set timeout function
which tries to do XMLHttpRequest.open("GET","some XML file URL").

Comment 3

[David :Bienvenu]

I believe that's a corrupt history file of some sort - I don't know what's
getting poked there but I can look into it (though handling of the corrupt
database is up to the mork client)

Comment 4

[Alec Flett]

well, what global history will do is try to open the file, and if it fails in
any way, then it blows away global history, and starts with a fresh one... so I
believe that nsGlobalHistory is doing the right thing

But I have to say, I don't see the harm in a few asserts if the file is corrupt.
Disk corruption is VERY rare, you (dmose) probably corrupted your history db by
running two instances of mozilla at the same time, which just isn't supported

Since the global history component does the right thing with mork DBs when they
are corrupt, I would say that the code is working as designed :)

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I was really proxy-filing this bug for Heikki, so it's really his call, but it
sounds like _something_ wasn't do the right thing with a corrupt db.  Heikki?

Comment 6

[David :Bienvenu]

The problem Heikki is seeing is a real problem, and should be fixed, but not by
removing the asserts.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Right.  I didn't mean to imply with the summary that the asserts were bad;
merely that they can't be a substitute for real error handling.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yeah, removing the asserts is not the right thing if you are supposed to not hit
them regularly (i.e. corrupted db -> cleanup -> no more assert). But since I
seem to consistently get the assert (and more asserts as time gooes by) it seems
like things aren't cleared as they should, or something else funky is going on.

I had had one or two asserts for a long time, then suddenly started gettings
dozens of asserts on first document load. Once I deleted history.dat the asserts
went away, but as soon as I opened my testcase I got one assert which I see on
first document load (now I am back to something like 2 or three asserts on the
first document load).

Comment 9

[David :Bienvenu]

I'm changing the summary - I've found the cause of this - someone is passing
history a null string, and history in turn is trying to set a mork column to the
null string, which mork doesn't like. I'll attach a fix.

Comment 10

[David :Bienvenu]

Attachment: proposed fix for one cause of assertion

Comment 11

[David :Bienvenu]

Does this fix the assertion you were seeing on the test case, Heikki? I don't
know what would cause more and more assertions to appear, but I can fix the
simple case of trying to load a file url causing an assertion.

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm... It still does not fix it for me. I'll attach my testcases.

Btw, you might want to replace the cast with .get() in the patch.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: tupshin.xul (save to local file system)

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: tupshin.js (save to local file system)

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, save the two attachments to local disc. Then start up your browser with URL
to tupshin.xul as the command line argument. You should see some debug dumps in
the console. Quit the application. Restart (same default arg). This time you
should get the mork assert.

Comment 16

[Alec Flett]

cool. sr=alecf if you indent the line below the if()

Comment 17

[Alec Flett]

oh, just saw that it doesn't fix it. 
david, you could just try fixing SetRowValue() for both PRUnichar* and char*

Comment 18

[David :Bienvenu]

So, Alec, I'm reluctant to change SetRow to simply ignore callers who pass the
null string because someone might want to empty out a column (i.e., set it to ""
when it used to be some other value). I could convert null to "", but then we'd
have the empty columns in the db (unless mork is smart enough to not write those
columns out - that's possible, actually). So, I was trying to fix the caller.

Heikki, exactly what should the command line arguments be? I saved tushpin.xul
and js to c:\tmp. should the command line be just c:\tmp\tushpin.xul, or
file://c:/tmp/tushpin.xul? I'll try the former.

Comment 19

[David :Bienvenu]

Heikki, I tried mozilla c:\tmp\tupshin.xul, and I didn't see any asserts, either
on the first run, or the second run. I haven't tried this without my patch, but
I can do so.

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I have used the file URL argument. I think you actually need (or at least you
needed that at some point) to have 3 slashes there...

file:///c:/tmp/tushpin.xul

I did not see the assertion on first launch, only on the second and subsequent
launches. But if you do not see the assertions with this testcase then I am
pretty much out of ideas...

Comment 21

[John Gaunt (redfive)]

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

Comment 22

[David :Bienvenu]

I think Alec fixed this - I no longer see the assert, anyway.

Comment 23

[[e-mail] (gone - use [e-mail] instead)]

C:\moz_src\mozilla\dist\WIN32_D.OBJ\bin>mozilla.exe 
file:///C:\WINNT\temp\tupshin.xul
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to c:\WINNT\Temp\leaklog.txt
Type Manifest File: C:\moz_src\mozilla\dist\WIN32_D.OBJ\bin\components\xpti.dat
nsNativeComponentLoader: autoregistering begins.
nsNativeComponentLoader: autoregistering succeeded
nNCL: registering deferred (0)
WEBSHELL+ = 1
WARNING: CSSLoaderImpl::LoadAgentSheet: Load of URL 'file:///C:/Documents%20and%
20Settings/Admi
 file c:\moz_src\mozilla\content\html\style\src\nsCSSLoader.cpp, line 1611
WARNING: CSSLoaderImpl::LoadAgentSheet: Load of URL 'file:///C:/Documents%20and%
20Settings/Admi
, file c:\moz_src\mozilla\content\html\style\src\nsCSSLoader.cpp, line 1611
WEBSHELL+ = 2
WARNING: unable to load UA style sheet, file 
c:\moz_src\mozilla\content\base\src\nsDocumentView
Note: verifyreflow is disabled
WARNING: unable to load UA style sheet, file 
c:\moz_src\mozilla\content\base\src\nsDocumentView
Note: styleverifytree is disabled
Note: frameverifytree is disabled
JavaScript error:
chrome://wallet/content/walletOverlay.js line 1: redeclaration of const hide

JavaScript error:
chrome://wallet/content/walletOverlay.js line 1: redeclaration of const hide

WEBSHELL+ = 3
JavaScript error:
 line 0: myhttprequest is not defined

Document file:///C%3A%5CWINNT%5Ctemp%5Ctupshin.xul loaded successfully
###!!! ASSERTION: cannot handle open-ended tag name query: 'Error', file 
c:\moz_src\mozilla\con
Start reading in bookmarks.html
Finished reading in bookmarks.html  (47000 microseconds)
WARNING: requested removal of nonexistent window
, file 
c:\moz_src\mozilla\embedding\components\windowwatcher\src\nsWindowWatcher.cpp, 
line 846
WEBSHELL- = 2
WEBSHELL- = 1
### nsCacheProfilePrefObserver::Observe [topic=xpcom-shutdown data=]
nsPluginHostImpl::Observe "xpcom-shutdown"

That's everything I see until all the annoying xpcom shutdown notices ;-)

This is on Windows 2000, debug build, fresh trunk.

I do not see any mork assertions whatsoever.

Verified worksforme.