Note: There are a few cases of duplicates in user autocompletion which are being worked on.

The bookmark tags feature is broken on Firefox 3.0rc1 for OpenSolaris

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Location Bar
--
major
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Alfred Peng, Assigned: Ginn Chen)

Tracking

Trunk
Firefox 3
x86
SunOS
Points:
---
Bug Flags:
blocking-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [RC2+][needs to land in mozilla-central])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 321497 [details]
tag missing for the debug build

I've tried on two different Firefox 3.0pre here on OpenSolaris 2008.05.
1. Mozilla/5.0 (X11; U; SunOS i86pc; zh-CN; rv:1.9pre) Gecko/2008050801 Minefield/3.0pre. Optimized build.
Reproduce steps:
1) open URL: www.google.com.
2) bookmark it with tag google.
3) type "google" in the location bar.

Firefox 3.0pre crashes with core stack below:
 fcd490e1 mutex_trylock_adaptive (8061fd8, 1) + 141
 fcd4a3ac mutex_lock_impl (8061fd8, 0) + dc
 fcd4a452 mutex_lock (8061fd8) + 10
 fe5d026c free     (8117c00) + d0
 fd6cdff2 void nsProfileLock::FatalSignalHandler(int) (b, 0, 80466d4) + 12e
 fcd5146f __sighndlr (b, 0, 80466d4, fd6cdec4) + f
 fcd46312 call_user_handler (b, 0, 80466d4) + 2bf
 fcd46546 sigacthandler (b, 0, 80466d4) + d0
 --- called from signal handler with signal 11 (SIGSEGV) ---
 fe5cea27 malloc   (c, fe4741be, 80469cc, fef8782a) + 307
 fef87854 void*operator new(unsigned) (c) + 38
 fe3f7f51 int nsStringArray::InsertStringAt(const nsAString_internal&,int) (ca69fc8, 8046a98, 
0) + 25
 fe29363a unsigned nsAutoCompleteSimpleResult::AppendMatch(const nsAString_internal&,const nsA
String_internal&,const nsAString_internal&,const nsAString_internal&) (ca69fc0, 8046a98, 8046e
1c, 804715c, 8046eb0) + 36
 fe28cce0 unsigned nsNavHistory::AutoCompleteProcessSearch(mozIStorageStatement*,const nsNavHi
story::QueryType,int*) (84d3000, a0d02b0, 1, 0) + aa4
 fe2898b8 void nsNavHistory::AutoCompleteTimerCallback(nsITimer*,void*) (975d5e0, 84d3000) + 2
b4
 fe44ec57 void nsTimerImpl::Fire() (975d5e0) + db
 fe44ed4b unsigned nsTimerEvent::Run() (a2416e0) + 53
 fe44b360 unsigned nsThread::ProcessNextEvent(int,int*) (8121e70, 1, 8047370) + 114
 fe3fa747 int NS_ProcessNextEvent_P(nsIThread*,int) (8121e70, 1) + 43
 fe2e2613 unsigned nsBaseAppShell::Run() (81b1fb0) + 37
 fe0cc505 unsigned nsAppStartup::Run() (8403880) + 31
 fd6c4340 XRE_main (3, 80477bc, 81170c0) + 1de4
 0805175c main     (3, 80477bc, 80477cc) + 23c
 0805141e _start   (3, 80478d4, 0, 0, 0, 8047923) + 7a

2. Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9pre) Gecko/2008051821 Minefield/3.0pre. Debug build. => Wrong behavior with the attached screenshot.
(Reporter)

Comment 1

9 years ago
Created attachment 321498 [details] [diff] [review]
patch v1

Also some error information in the terminal for the debug build as below:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "title.match(/^(.+) \u2013 (.+)$/) is null" {file: "chrome://global/content/bindings/autocomplete.xml" line: 1337}]' when calling method: [nsIAutoCompletePopup::invalidate]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

The patch works on my box here. Could it be the fix?
Attachment #321498 - Flags: review?(edilee)

Comment 2

9 years ago
Alfred: What happens when you go to this page:
javascript:'\u2013'.match(/\u2013/)

Also, you said there's a crash or does it just not show the right title/tag? The screenshot seems to show some result with tag missing.. it's in the process of crashing? I'm assuming sunweb.central is the bookmark title and the tag is something else that isn't shown?
(Reporter)

Comment 3

9 years ago
Created attachment 321544 [details]
bookmarks with title and tag for the debug build

(In reply to comment #2)
> Alfred: What happens when you go to this page:
> javascript:'\u2013'.match(/\u2013/)
> 
The output is "–" in this case.

> Also, you said there's a crash or does it just not show the right title/tag?
>
With optimized build, I can get a crash sometimes and sometimes hang. For debug build, I don't have a crash yet. Just the title/tag is wrong and error message in the terminal.

> The screenshot seems to show some result with tag missing.. it's in the 
> process of crashing?
>
No. The screenshot is token for the debug build. Just tag missing, no crash.

> I'm assuming sunweb.central is the bookmark title and the tag is
> something else that isn't shown?
> 
Attached is the bookmark information for my debug build. The tag for sunweb.central is sun in this case.
(Reporter)

Updated

9 years ago
Attachment #321497 - Attachment description: screenshot → tag missing for the debug build
(Reporter)

Updated

9 years ago
Attachment #321544 - Attachment description: bookmarks with title and tag → bookmarks with title and tag for the debug build

Comment 4

9 years ago
(In reply to comment #1)
> The patch works on my box here. Could it be the fix?
Works how so? Does the UI show the page title and tags correctly? I'm not sure how a plain u2013 in the regular expression would match the ndash.
(Assignee)

Comment 5

9 years ago
I'm not afraid this is not the right fix for the crash.
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> I'm not afraid this is not the right fix for the crash.
> 
typo.
I'm afraid....
(Reporter)

Comment 7

9 years ago
Created attachment 321556 [details]
bookmarks with title and tag after patching

(In reply to comment #4)
> Works how so? Does the UI show the page title and tags correctly? I'm not sure
> how a plain u2013 in the regular expression would match the ndash.
> 
hrmm, the root cause might be hidden somewhere else?

Comment 8

9 years ago
the specific crash is kinda funny.

malloc grabbed a lock and then crashed.
the crash handler caught the crash
 it then might have tried to malloc (and suceeded?)
 it then tried to free, and this failed, probably because of the lock held by the original malloc (at least that's my guess)

it doesn't matter, what's interesting is the part here:
 fe5cea27 malloc   (c, fe4741be, 80469cc, fef8782a) + 307
 fef87854 void*operator new(unsigned) (c) + 38
 fe3f7f51 int nsStringArray::InsertStringAt(const nsAString_internal&,int)
(ca69fc8, 8046a98, 0) + 25

what size was nsStringArray::InsertStringAt actually allocating?
(Assignee)

Comment 9

9 years ago
timeless,
it's a typical "crash" stack of memory corruption.

796 nsStringArray::InsertStringAt(const nsAString& aString, PRInt32 aIndex)
797 {
798   nsString* string = new nsString(aString);

and
sizeof (nsString) = 0xc

so I don't think jemalloc is wrong.
(Assignee)

Comment 10

9 years ago
I suspect the crash is a bug of the compiler - Sun Studio 11.

777       style = showTags ? NS_LITERAL_STRING("tag") : (parentId &&
778         !mLivemarkFeedItemIds.Get(parentId, &dummy)) ||
779         mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ?
780         NS_LITERAL_STRING("bookmark") : NS_LITERAL_STRING("favicon");

This line confused the compiler.
When showTags is true, it called string constructor for "tag" but didn't  for "bookmark" and "favicon".
But then it called deconstructor for local pointer of them all.

I changed the code to
nsString _tag = NS_LITERAL_STRING("tag");
nsString _bookmark = NS_LITERAL_STRING("bookmark");
nsString _favicon = NS_LITERAL_STRING("favicon");
style = showTags ? _tag : (parentId &&
       !mLivemarkFeedItemIds.Get(parentId, &dummy)) ||
        mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ?
        _bookmark : _favicon;

The crash is gone.
And if I use Sun Studio 12, the crash is gone, too.

So I don't think we need to worry about it, because we will switch to Sun Studio 12 very soon.


The title & tag no show issue in the screenshot is not related to the crash, and need to be fixed.
(Assignee)

Comment 11

9 years ago
Comment on attachment 321498 [details] [diff] [review]
patch v1

This is not  the right fix.

I think the root problem here is
NS_NAMED_LITERAL_STRING(kTitleTagsSeparator, " \u2013 ");
is not portable.

On Solaris, kTitleTagsSeparator is a unicode string of plain ASCII " \u2013 ", not converted.
Attachment #321498 - Flags: review?(edilee) → review-
(Reporter)

Comment 12

9 years ago
The problem seems to be related to the missing of HAVE_CPP_2BYTE_WCHAR_T on Solaris: http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsStringAPI.h#1052.
(Assignee)

Comment 13

9 years ago
Created attachment 321584 [details] [diff] [review]
patch

I don't know what's the best way to do this.
This works for me.

The following also works.
const PRUnichar kTitleTagsSeparator[] = { ' ', 0x2013, ' ', '\0' };
But you need to use
 title += nsAutoString(kTitleTagsSeparator) + entryTags;
Assignee: nobody → ginn.chen
Attachment #321498 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #321584 - Flags: review?(edilee)

Comment 14

9 years ago
Comment on attachment 321584 [details] [diff] [review]
patch

Let's go with your other suggestion with the array. I'll attach the updated patch.

I'm assuming unit tests failed when compiling on Solaris? Pretty much any of these tests in the following directory:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/tests/autocomplete/
Attachment #321584 - Flags: review?(edilee) → review-

Comment 15

9 years ago
Created attachment 321603 [details] [diff] [review]
v2

Going with the alternate solution from comment 13:
> const PRUnichar kTitleTagsSeparator[] = { ' ', 0x2013, ' ', '\0' };
> But you need to use
>  title += nsAutoString(kTitleTagsSeparator) + entryTags;
But keeping the changes to the kTitleTagsSeparator at the top.

Ginn, this works for you, yeah? Compiles and passes unit tests? Passes all tests for me on OS X.

r=Mardak, requesting r?dietrich for official review

Is this something needed for Firefox 3 or will Sun be providing builds of Firefox 3 with custom patches? (I'm not familiar with how those releases are handled.)
Attachment #321603 - Flags: review?(dietrich)
(Reporter)

Comment 16

9 years ago
(In reply to comment #15)
> Ginn, this works for you, yeah? Compiles and passes unit tests? Passes all
> tests for me on OS X.
> 
The patch works for me on OpenSolaris 2008.05. Any pointer on how to run the unit tests?

> Is this something needed for Firefox 3 or will Sun be providing builds of
> Firefox 3 with custom patches? (I'm not familiar with how those releases are
> handled.)
> 
We'd try not to hold any patch on our side, especially for the OpenSolaris contributed builds.

Mike, do we still have a chance to land this patch to 1.9.0 branch tree before 3.0 is released? Do we have a RC2(RC1 is code-freezed as I know) to include this patch? Or even the patch for bug 422055? Otherwise, we'll have to hold them and wait for the security updated.

Comment 17

9 years ago
Alfred: http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests
Easiest way would probably to do..

make -C objdir/toolkit/components/places check
(Reporter)

Comment 18

9 years ago
Mardak, the tests got PASS with the patch on OpenSolaris 2008.05.

Comment 19

9 years ago
Just a sanity check, the tests did fail without the patch?
(Reporter)

Comment 20

9 years ago
Created attachment 321895 [details]
test fail log

(In reply to comment #19)
> Just a sanity check, the tests did fail without the patch?
> 

FAILED for some tests.

Comment 21

9 years ago
Comment on attachment 321895 [details]
test fail log

>-n ../../../../_tests/xpcshell-simple/test_places/autocomplete/test_416211.js: 
>FAIL
>*** CHECK FAILED: Didn't find the current result (http://theuri/, Bookmark title u2013 superTag) in expected: 0
Thanks for running it. Yup, the autocomplete results are coming back with plain text "u2013" instead of ndash.

Comment 22

9 years ago
(I suppose this also means we don't need to write a testcase for this bug.)
Flags: in-testsuite+

Updated

9 years ago
Whiteboard: [RC2?][has patch][need dietrich review][has Mardak review][has tests]

Updated

9 years ago
Attachment #321603 - Flags: review+
(Reporter)

Updated

9 years ago
Summary: Firefox 3.0pre crashes with auto completion for tagged bookmark → The bookmark tags feature is broken on Firefox 3.0rc1 for OpenSolaris
Comment on attachment 321603 [details] [diff] [review]
v2

r=me
Attachment #321603 - Flags: review?(dietrich) → review+

Updated

9 years ago
Whiteboard: [RC2?][has patch][need dietrich review][has Mardak review][has tests] → [RC2?][has patch][has dietrich review][has Mardak review][has tests]

Comment 24

9 years ago
Dietrich and Ed is this safe enough and severe enough to take in a possible RC2?

Comment 25

9 years ago
I would say it's pretty safe -- passes tests on solaris and os x; and makes tagging work with the location bar (no more crashes!)
Flags: blocking-firefox3+
Comment on attachment 321603 [details] [diff] [review]
v2

Get this landed ASAP on CVS, svp!
Attachment #321603 - Flags: approval1.9+

Comment 27

9 years ago
user:        Edward Lee <edward.lee@engineering.uiuc.edu>
date:        Mon May 26 16:20:16 2008 -0500
summary:     Bug 434340 - Firefox 3.0pre crashes with auto completion for tagged bookmark. p=ginn.chen@sun.com (Ginn Chen), r=Mardak, r=dietrich, b-ff3=beltzner, a1.9=beltzner

http://hg.mozilla.org/cvs-trunk-mirror/index.cgi/rev/7af4b1ad0b2e

Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.59; previous revision: 1.58
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [RC2?][has patch][has dietrich review][has Mardak review][has tests] → [RC2+][needs to land in mozilla-central]
Target Milestone: --- → Firefox 3
Did this cause the Rlk tinderbox orange?
Can someone that runs solaris please verify this bug?  thanks.
(Reporter)

Comment 30

9 years ago
Verified on my OpenSolaris 2008.05 with Firefox 3.0 nightly: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9pre) Gecko/2008060401 Minefield/3.0pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.