Last Comment Bug 434340 - The bookmark tags feature is broken on Firefox 3.0rc1 for OpenSolaris
: The bookmark tags feature is broken on Firefox 3.0rc1 for OpenSolaris
Status: VERIFIED FIXED
[RC2+][needs to land in mozilla-central]
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: x86 SunOS
: -- major (vote)
: Firefox 3
Assigned To: Ginn Chen
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-18 10:03 PDT by Alfred Peng
Modified: 2008-06-03 18:47 PDT (History)
8 users (show)
mbeltzner: blocking‑firefox3+
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
tag missing for the debug build (32.15 KB, image/png)
2008-05-18 10:03 PDT, Alfred Peng
no flags Details
patch v1 (1.19 KB, patch)
2008-05-18 10:07 PDT, Alfred Peng
ginn.chen: review-
Details | Diff | Splinter Review
bookmarks with title and tag for the debug build (17.67 KB, image/png)
2008-05-18 18:37 PDT, Alfred Peng
no flags Details
bookmarks with title and tag after patching (28.21 KB, image/png)
2008-05-18 23:12 PDT, Alfred Peng
no flags Details
patch (1.42 KB, patch)
2008-05-19 05:32 PDT, Ginn Chen
edilee: review-
Details | Diff | Splinter Review
v2 (1.54 KB, patch)
2008-05-19 06:48 PDT, Ed Lee :Mardak
dietrich: review+
edilee: review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
test fail log (6.25 KB, text/plain)
2008-05-20 21:10 PDT, Alfred Peng
no flags Details

Description Alfred Peng 2008-05-18 10:03:46 PDT
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.
Comment 1 Alfred Peng 2008-05-18 10:07:18 PDT
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?
Comment 2 Ed Lee :Mardak 2008-05-18 13:52:08 PDT
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 2008-05-18 18:37:33 PDT
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.
Comment 4 Ed Lee :Mardak 2008-05-18 22:38:05 PDT
(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 2008-05-18 22:44:47 PDT
I'm not afraid this is not the right fix for the crash.
Comment 6 Ginn Chen 2008-05-18 22:55:42 PDT
(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 2008-05-18 23:12:28 PDT
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 timeless 2008-05-19 00:18:10 PDT
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 2008-05-19 00:26:13 PDT
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 2008-05-19 03:17:58 PDT
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 2008-05-19 04:47:18 PDT
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 2008-05-19 04:53:52 PDT
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 2008-05-19 05:32:40 PDT
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;
Comment 14 Ed Lee :Mardak 2008-05-19 06:42:59 PDT
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 2008-05-19 06:48:49 PDT
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.)
Comment 16 Alfred Peng 2008-05-19 19:41:39 PDT
(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 2008-05-19 20:24:05 PDT
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 2008-05-20 20:27:11 PDT
Mardak, the tests got PASS with the patch on OpenSolaris 2008.05.
Comment 19 Ed Lee :Mardak 2008-05-20 20:34:30 PDT
Just a sanity check, the tests did fail without the patch?
Comment 20 Alfred Peng 2008-05-20 21:10:21 PDT
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 Ed Lee :Mardak 2008-05-20 21:25:14 PDT
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 2008-05-20 21:25:58 PDT
(I suppose this also means we don't need to write a testcase for this bug.)
Comment 23 Dietrich Ayala (:dietrich) 2008-05-23 09:33:41 PDT
Comment on attachment 321603 [details] [diff] [review]
v2

r=me
Comment 24 Mike Schroepfer 2008-05-23 11:28:55 PDT
Dietrich and Ed is this safe enough and severe enough to take in a possible RC2?
Comment 25 Ed Lee :Mardak 2008-05-23 11:51:49 PDT
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] 2008-05-26 13:54:57 PDT
Comment on attachment 321603 [details] [diff] [review]
v2

Get this landed ASAP on CVS, svp!
Comment 27 Ed Lee :Mardak 2008-05-26 15:27:11 PDT
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
Comment 28 Mats Palmgren (vacation) 2008-05-26 16:17:07 PDT
Did this cause the Rlk tinderbox orange?
Comment 29 Marcia Knous [:marcia - use ni] 2008-06-03 11:08:51 PDT
Can someone that runs solaris please verify this bug?  thanks.
Comment 30 Alfred Peng 2008-06-03 18:47:45 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.