335558 - Make storage form history use Mork if available

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P1)

Description

[Brett Wilson]

Currently, it is not possible to build storage + mork like we are planning to do on branch. Since the Mork version is better tested, we should pick that one.

Comment 1

[Brett Wilson]

Attachment: Patch

Please check carefully, I can't test all possibilities, and I remember problems with the satchel build before (it's stupidly complicated).

Comment 2

[Benjamin Smedberg]

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

brettw, mconnor posted a patch for this in the original bug... you don't need to ifdef REQUIRES at all, since they just specify directories to search for header files.

Comment 3

[Brett Wilson]

OK, I'll just mark this as a dupe.

*** This bug has been marked as a duplicate of 335377 ***

Comment 4

[Brett Wilson]

Attachment: Satchel keyed of MOZ_PLACES

Comment 5

[Benjamin Smedberg]

Comment on attachment 221509 [details] [diff] [review]
Satchel keyed of MOZ_PLACES

I assume we're taking this for a2?

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 221509 [details] [diff] [review]
Satchel keyed of MOZ_PLACES

let's get this in ASAP

Comment 7

[Brett Wilson]

Fixed on trunk and 1.8 branch.

Comment 8

[Stefan Sitter [:ssitter]]

This check in looks like the cause for solaria tinderbox bustage:
(http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird)

.../mozilla/toolkit/components/satchel/src/nsFormHistory.cpp(946) : 
error C2061: syntax error : identifier 'littleEndianByteOrder'

.../mozilla/toolkit/components/satchel/src/nsFormHistory.cpp(975) : 
error C2664: 'SaveByteOrder' : cannot convert parameter 1 from 'class nsAutoString &(void)' to 'const class nsAString_internal &'
        Context does not allow for disambiguation of overloaded function

.../mozilla/toolkit/components/satchel/src/nsFormHistory.cpp(979) : 
error C2664: 'int __fastcall __fastcall nsSubstring::Equals(const class nsSubstring &) const' : cannot convert parameter 1 from 'class nsAutoString &(__cdecl *)(void)' to 'const class nsSubstring &'
        Reason: cannot convert from 'class nsAutoString &(__cdecl *)(void)' to 'const class nsSubstring'
        No constructor could take the source type, or constructor overload resolution was ambiguous
make[6]: *** [nsFormHistory.obj] Error 2

Comment 9

[Doug Turner (:dougt)]

this causes bug 340004.  reopening.  

Comment 10

[Doug Turner (:dougt)]

The requires section of the makefile need not be change.  the diff adds two additional requirement that only matter when not building MOZ_PLACES.

I am wondering if the extra requires causes us to grab the wrong header when not building Places?

I did something like this, and it seams to work for me:

Index: components/satchel/src/Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/toolkit/components/satchel/src/Makefile.in,v
retrieving revision 1.8.8.2
diff -u -1 -0 -r1.8.8.2 Makefile.in
--- components/satchel/src/Makefile.in	30 May 2006 22:41:59 -0000	1.8.8.2
+++ components/satchel/src/Makefile.in	1 Jun 2006 23:47:03 -0000
@@ -58,28 +58,27 @@
 		docshell \
 		gfx \
 		necko \
 		widget \
 		content \
 		view \
 		locale \
 		unicharutil \
 		intl \
 		pref \
-		storage \
-		morkreader \
 		$(NULL)
 
 CPPSRCS = nsFormFillController.cpp \
           $(NULL)
 
 ifdef MOZ_PLACES
+REQUIRES += storage morkreader
 CPPSRCS += nsStorageFormHistory.cpp
 else
 REQUIRES += mork
 CPPSRCS += nsFormHistory.cpp
 endif
 
 LOCAL_INCLUDES = \
 		-I$(srcdir)/../../passwordmgr/base \
 		-I$(srcdir)/../../build \
 		$(NULL)

Comment 11

[Darin Fisher]

doug, r=me for that change.

Comment 12

[Doug Turner (:dougt)]

Checking in Makefile.in;
/cvsroot/mozilla/toolkit/components/satchel/src/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done

Checking in Makefile.in;
/cvsroot/mozilla/toolkit/components/satchel/src/Makefile.in,v  <--  Makefile.in
new revision: 1.8.8.3; previous revision: 1.8.8.2
done


I checked in this change.  (r=darin) not sure if fixes the issue, but it seams to for my local build.  I am kicking my tinderboxes now.

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

Branch nightlies of Firefox have been crashing a lot since the initial fix from this bug. Bug 339861 was filed for this. There are plenty of Talkback reports and some further information there. Please take a look.

Comment 14

[Doug Turner (:dougt)]

it sounds like the above makefile fix does bandaide the crash (see https://bugzilla.mozilla.org/show_bug.cgi?id=339861#c21).

Comment 15

[Brett Wilson]

The only reason for using the Mork one is that it is better tested than the storage one. However, if the Mork version is causing problems, perhaps we should just ship with storage? It could also be our storage canary for Places.

Comment 16

[Doug Turner (:dougt)]

size is an issue too right?

Comment 17

[Brett Wilson]

(In reply to comment #16)
> size is an issue too right?

We're already shipping mozStorage with Firefox 2 (DOM Storage and safe browsing use it), so size isn't an issue.

Comment 18

[Doug Turner (:dougt)]

sorry brett, i think i misunderstood you.  I am all for shipping less.  shipping both sqlite and dbm seams silly.

Comment 19

[Brett Wilson]

(In reply to comment #18)
> sorry brett, i think i misunderstood you.  I am all for shipping less. 
> shipping both sqlite and dbm seams silly.

We're shipping both Mork and sqlite in Firefox 2 regardless of what we do with form history. GlobalHistory requires Mork.

Comment 20

[Brett Wilson]

I would be very surprised that this crash was caused by this because it just re-enables the old code. There have been several checkins to FormHistory when it was disabled in Firefox that could have introduced problems that have gone undetected until it was re-enabled:
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Ftoolkit%2Fcomponents%2Fsatchel%2Fsrc&file=nsFormHistory.cpp&rev1=1.31&rev2=1.25&whitespace_mode=show&diff_mode=context

I was seeing this crash before, but now that I'm trying to fix it, it doesn't happen anymore. :( If it's consistently happening for somebody, what happens if you revert to a version of nsFormHistory.cpp/h from last summer?

Comment 21

[Brett Wilson]

(In reply to comment #8)
> This check in looks like the cause for solaria tinderbox bustage:
> (http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird)

This happened because a change was checked in to the Mork code when it was disabled. It's valid code, though, but it doesn't work on old compilers. Solaria uses VC98 which is unsupported on trunk (see bug 340192).

Comment 22

[Brett Wilson]

Attachment: Key satchel off Storage again

This isn't a straight backout of the code, it just replaces the PLACES keys with STORAGE, which is cleaner than the old code.

Comment 23

[Brett Wilson]

Me and bryner's feeling about this is that by covering the problem by moving back to storage is not the correct approach.

From my dev.apps.firefox post:

This is a huge problem, and I've been so far unable to reproduce it. It seems that basically nobody has done any testing. Hopefully this post will cause somebody that does see this problem to help out.

Since this just re-enabled old working code, I suspect that one of the changes checked in on or after Storage autocomplete was first enabled is causing the problem. We would not have noticed the problems because it was disabled.

There are two options:

Option 1: Revert back to Storage form autocomplete. This should immediately fix the problem, but will ensure that Mork autocomplete will stay permanently broken because nobody is testing it. The storage autocomplete is also not as well tested, and needs some work before release for some things like recovering in case of a corrupted database (currently it will just be permanently broken).

Option 2: Find the problem and fix the Mork version. This has the disadvantage of taking longer and pissing people off.

I'm OK with either of these options. However, if we do (1), I feel that we should delete Mork autocomplete from toolkit, which I imagine people will be opposed to.

Comment 24

[Benjamin Smedberg]

I don't know who "people" is, but as toolkit module owner I'm not opposed to it.

Comment 25

[Brett Wilson]

I can reproduce this on a windows nightly. I'm going to try to get my own Windows build going, but it will take several hours.

Comment 26

[Brett Wilson]

(In reply to comment #24)
> I don't know who "people" is, but as toolkit module owner I'm not opposed to
> it.

Ok, this means that any app that wants for history must also ship Storage. This is certainly easiest for me.

Comment 27

[Brett Wilson]

See bug 340871 for removing Mork formhistory.

Comment 28

[Brett Wilson]

Attachment: Patch for crash

Comment 29

[Brett Wilson]

Attachment: Trunk patch for problems

This is a trunk patch. There might be a related problem, although it will not be visible by default because the storage version is used.

Comment 30

[Brett Wilson]

Branch patch is up on the 1.8 branch, leaving open for trunk fix.

Comment 31

[Brett Wilson]

Trunk patch in.