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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Alfred Peng]

Attachment: 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.

Comment 1

[Alfred Peng]

Attachment: 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?

Comment 2

[Ed Lee :Mardak]

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?

Comment 3

[Alfred Peng]

Attachment: 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.

Comment 4

[Ed Lee :Mardak]

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

Comment 5

[Ginn Chen]

I'm not afraid this is not the right fix for the crash.

Comment 6

[Ginn Chen]

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

Comment 7

[Alfred Peng]

Attachment: 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

[timeless]

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?

Comment 9

[Ginn Chen]

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.

Comment 10

[Ginn Chen]

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.

Comment 11

[Ginn Chen]

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.

Comment 12

[Alfred Peng]

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.

Comment 13

[Ginn Chen]

Attachment: 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;

Comment 14

[Ed Lee :Mardak]

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/

Comment 15

[Ed Lee :Mardak]

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

Comment 16

[Alfred Peng]

(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

[Ed Lee :Mardak]

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

Comment 18

[Alfred Peng]

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

Comment 19

[Ed Lee :Mardak]

Just a sanity check, the tests did fail without the patch?

Comment 20

[Alfred Peng]

Attachment: 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

[Ed Lee :Mardak]

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

[Ed Lee :Mardak]

(I suppose this also means we don't need to write a testcase for this bug.)

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 321603 [details] [diff] [review]
v2

r=me

Comment 24

[Mike Schroepfer]

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

Comment 25

[Ed Lee :Mardak]

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

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 321603 [details] [diff] [review]
v2

Get this landed ASAP on CVS, svp!

Comment 27

[Ed Lee :Mardak]

user:        Edward Lee <[e-mail]>
date:        Mon May 26 16:20:16 2008 -0500
summary:     Bug 434340 - Firefox 3.0pre crashes with auto completion for tagged bookmark. p=[e-mail] (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

Comment 28

[Mats Palmgren (inactive)]

Did this cause the Rlk tinderbox orange?

Comment 29

[Marcia Knous [:marcia]]

Can someone that runs solaris please verify this bug?  thanks.

Comment 30

[Alfred Peng]

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.