Closed
Bug 335558
Opened 18 years ago
Closed 18 years ago
Make storage form history use Mork if available
Categories
(Toolkit :: Form Manager, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta1
People
(Reporter: brettw, Assigned: brettw)
Details
(Keywords: fixed1.8.1)
Attachments
(5 files)
5.31 KB,
patch
|
benjamin
:
first-review-
|
Details | Diff | Splinter Review |
2.99 KB,
patch
|
benjamin
:
first-review+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
Details | Diff | Splinter Review | |
1000 bytes,
patch
|
bryner
:
first-review+
bryner
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
1.65 KB,
patch
|
bryner
:
first-review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•18 years ago
|
Component: Places → Satchel
Product: Firefox → Toolkit
Target Milestone: --- → mozilla1.8.1alpha2
Assignee | ||
Comment 1•18 years ago
|
||
Please check carefully, I can't test all possibilities, and I remember problems with the satchel build before (it's stupidly complicated).
Attachment #219905 -
Flags: first-review?(bryner)
Comment 2•18 years ago
|
||
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.
Attachment #219905 -
Flags: first-review?(bryner) → first-review-
Assignee | ||
Comment 3•18 years ago
|
||
OK, I'll just mark this as a dupe. *** This bug has been marked as a duplicate of 335377 ***
Assignee | ||
Updated•18 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 4•18 years ago
|
||
Attachment #221509 -
Flags: first-review?
Assignee | ||
Updated•18 years ago
|
Attachment #221509 -
Flags: first-review? → first-review?(benjamin)
Comment 5•18 years ago
|
||
Comment on attachment 221509 [details] [diff] [review] Satchel keyed of MOZ_PLACES I assume we're taking this for a2?
Attachment #221509 -
Flags: first-review?(benjamin)
Attachment #221509 -
Flags: first-review+
Attachment #221509 -
Flags: approval-branch-1.8.1?(mtschrep)
Assignee | ||
Updated•18 years ago
|
Priority: -- → P1
Target Milestone: mozilla1.8.1alpha2 → mozilla1.8.1beta1
Assignee | ||
Updated•18 years ago
|
Whiteboard: has patch
Updated•18 years ago
|
Flags: blocking-firefox2?
Version: Trunk → 1.8 Branch
Updated•18 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Comment 6•18 years ago
|
||
Comment on attachment 221509 [details] [diff] [review] Satchel keyed of MOZ_PLACES let's get this in ASAP
Attachment #221509 -
Flags: approval-branch-1.8.1?(mtschrep) → approval-branch-1.8.1+
Assignee | ||
Comment 7•18 years ago
|
||
Fixed on trunk and 1.8 branch.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: has patch
Comment 8•18 years ago
|
||
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•18 years ago
|
||
this causes bug 340004. reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•18 years ago
|
||
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•18 years ago
|
||
doug, r=me for that change.
Comment 12•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
it sounds like the above makefile fix does bandaide the crash (see https://bugzilla.mozilla.org/show_bug.cgi?id=339861#c21).
Assignee | ||
Comment 15•18 years ago
|
||
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.
No longer depends on: 339861
Comment 16•18 years ago
|
||
size is an issue too right?
Assignee | ||
Comment 17•18 years ago
|
||
(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•18 years ago
|
||
sorry brett, i think i misunderstood you. I am all for shipping less. shipping both sqlite and dbm seams silly.
Assignee | ||
Comment 19•18 years ago
|
||
(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.
Assignee | ||
Comment 20•18 years ago
|
||
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?
Assignee | ||
Comment 21•18 years ago
|
||
(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).
Assignee | ||
Comment 22•18 years ago
|
||
This isn't a straight backout of the code, it just replaces the PLACES keys with STORAGE, which is cleaner than the old code.
Attachment #224762 -
Flags: first-review?
Attachment #224762 -
Flags: approval-branch-1.8.1?
Assignee | ||
Updated•18 years ago
|
Attachment #224762 -
Flags: first-review?(mconnor)
Attachment #224762 -
Flags: first-review?
Attachment #224762 -
Flags: approval-branch-1.8.1?(mconnor)
Attachment #224762 -
Flags: approval-branch-1.8.1?
Assignee | ||
Updated•18 years ago
|
Attachment #224762 -
Flags: first-review?(mconnor)
Attachment #224762 -
Flags: approval-branch-1.8.1?(mconnor)
Assignee | ||
Comment 23•18 years ago
|
||
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•18 years ago
|
||
I don't know who "people" is, but as toolkit module owner I'm not opposed to it.
Assignee | ||
Comment 25•18 years ago
|
||
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.
Assignee | ||
Comment 26•18 years ago
|
||
(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.
Assignee | ||
Comment 27•18 years ago
|
||
See bug 340871 for removing Mork formhistory.
Updated•18 years ago
|
QA Contact: places → satchel
Assignee | ||
Comment 28•18 years ago
|
||
Attachment #225071 -
Flags: first-review?(bryner)
Attachment #225071 -
Flags: approval-branch-1.8.1?(bryner)
Updated•18 years ago
|
Attachment #225071 -
Flags: first-review?(bryner)
Attachment #225071 -
Flags: first-review+
Attachment #225071 -
Flags: approval-branch-1.8.1?(bryner)
Attachment #225071 -
Flags: approval-branch-1.8.1+
Assignee | ||
Comment 29•18 years ago
|
||
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.
Attachment #225076 -
Flags: first-review?(bryner)
Assignee | ||
Comment 30•18 years ago
|
||
Branch patch is up on the 1.8 branch, leaving open for trunk fix.
Updated•18 years ago
|
Attachment #225076 -
Flags: first-review?(bryner) → first-review+
Assignee | ||
Comment 31•18 years ago
|
||
Trunk patch in.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Component: Satchel → Form Manager
You need to log in
before you can comment on or make changes to this bug.
Description
•