Last Comment Bug 621009 - Crash with moz-icon url [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ][@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ][@ mozilla::places::URIBinder::Bind]
: Crash with moz-icon url [@ nsTHashtable<mozilla::places::History::KeyClass>::...
Status: RESOLVED FIXED
[places-next-wanted]
: crash, regression
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: mozilla5
Assigned To: Marco Bonardo [::mak]
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks: 560198
  Show dependency treegraph
 
Reported: 2010-12-22 11:39 PST by Pascal Chevrel:pascalc
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch v1.0 (1.58 KB, patch)
2011-03-07 06:03 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (1.49 KB, patch)
2011-03-07 06:14 PST, Marco Bonardo [::mak]
joe: review+
Details | Diff | Splinter Review
patch v1.2 (1.49 KB, patch)
2011-03-11 06:34 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Pascal Chevrel:pascalc 2010-12-22 11:39:10 PST
Mozilla/5.0 (X11; Linux i686; rv:2.0b9pre) Gecko/20101222 Firefox/4.0b9pre

1/ type moz-icon://stock/?size=toolbar&state=disabled in the location bar
2/ hit enter

result: Firefox crashes
crash id: bp-b125a1b8-d557-490e-82fb-407d12101222
Reproducible: always
Fx 3.6.13 does not crashes
Comment 1 timeless 2010-12-22 11:54:50 PST
Signature nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey 
UUID b125a1b8-d557-490e-82fb-407d12101222 
Time 2010-12-22 11:32:34.425479 
Uptime 70 
Last Crash 74 seconds before submission 
Install Age 28736 seconds (8.0 hours) since version was first installed. 
Product Firefox 
Version 4.0b9pre 
Build ID 20101222030351 
Branch 2.0 
OS Linux 
OS Version 0.0.0 Linux 2.6.35-24-generic #42-Ubuntu SMP Thu Dec 2 01:41:57 UTC 2010 i686 
CPU x86 
CPU Info GenuineIntel family 6 model 23 stepping 6 
Crash Reason SIGSEGV 
Crash Address 0x0 
User Comments 
Processor Notes 
EMCheckCompatibility False 
Crashing ThreadFrame Module Signature [Expand] Source 
0 libxul.so nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey nsURIHashKey.h:74 
1 libxul.so PL_DHashTableOperate pldhash.c:615 
2 libxul.so mozilla::places::History::NotifyVisited nsTHashtable.h:170 
3 libxul.so mozilla::places::::NotifyVisitObservers::Run History.cpp:257 
4 libxul.so nsThread::ProcessNextEvent nsThread.cpp:626 
5 libxul.so NS_ProcessNextEvent_P nsThreadUtils.cpp:250 
6 libxul.so mozilla::ipc::MessagePump::Run MessagePump.cpp:110 
7 libxul.so MessageLoop::RunInternal message_loop.cc:219 
8 libxul.so MessageLoop::Run message_loop.cc:202 
9 libxul.so nsBaseAppShell::Run nsBaseAppShell.cpp:192 
10 libxul.so nsAppStartup::Run nsAppStartup.cpp:191 
11 @0x2030877 
12 libxul.so XRE_main nsAppRunner.cpp:3694 
13 firefox-bin main nsBrowserApp.cpp:158 
14 libc-2.12.1.so libc-2.12.1.so@0x16ce6 
15 firefox-bin firefox-bin@0x1390 
16 firefox-bin Output nsBrowserApp.cpp:77
Comment 2 Pascal Chevrel:pascalc 2010-12-22 12:08:25 PST
Last good nightly: 2010-12-15 First bad nightly: 2010-12-16

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66036625795f&tochange=f11f7ed625ba
Comment 3 Marco Bonardo [::mak] 2011-03-04 06:19:18 PST
The crash is reproduceable on RC nightly (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre), the steps in comment 0 are still good to reproduce (even if the regressionwindow makes no sense) but the stack is now different and involves AsyncGetBookmarksForURI.

https://crash-stats.mozilla.com/report/index/bp-86083e73-b5b3-464f-8977-9404c2110304

0|0|xul.dll|mozilla::places::URIBinder::Bind(mozIStorageStatement *,nsACString_internal const &,nsIURI *)|hg:hg.mozilla.org/mozilla-central:toolkit/components/places/src/Helpers.cpp:290712e55ade|139|0x12 0|1|xul.dll|`anonymous namespace'::AsyncGetBookmarksForURI<void ( nsNavBookmarks::*)(mozilla::places::ItemVisitData const &),mozilla::places::ItemVisitData>::Init()|hg:hg.mozilla.org/mozilla-central:toolkit/components/places/src/nsNavBookmarks.cpp:290712e55ade|139|0x1f 0|2|xul.dll|nsNavBookmarks::OnVisit(nsIURI *,__int64,__int64,__int64,__int64,unsigned int,unsigned int *)|hg:hg.mozilla.org/mozilla-central:toolkit/components/places/src/nsNavBookmarks.cpp:290712e55ade|2977|0x6 0|3|xul.dll|nsNavHistory::NotifyOnVisit(nsIURI *,__int64,__int64,__int64,__int64,int)|hg:hg.mozilla.org/mozilla-central:toolkit/components/places/src/nsNavHistory.cpp:290712e55ade|2075|0x89 0|4|xul.dll|mozilla::places::`anonymous namespace'::NotifyVisitObservers::Run()|hg:hg.mozilla.org/mozilla-central:toolkit/components/places/src/History.cpp:290712e55ade|471|0x26 0|5|xul.dll|nsThread::ProcessNextEvent(int,int *)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:290712e55ade|633|0x13 0|6|xul.dll|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:290712e55ade|110|0x28
Comment 4 Marco Bonardo [::mak] 2011-03-04 06:19:36 PST
taking because sounds like could be important
Comment 5 Marco Bonardo [::mak] 2011-03-05 04:18:45 PST
Interesting, NS_NewURI fails but we don't error check it because we extracted the same spec from a nsIURI. the extracted spec is "moz-icon://?size=toolbar&state=disabled" though and not the original one.
Comment 6 Marco Bonardo [::mak] 2011-03-05 04:51:56 PST
So, nsMozIconURI acts wrongly on this specific URI, the correct form is:
moz-icon://stock/[icon identifier]?[attributes]
while in this case we have:
moz-icon://stock/?[attributes]

mStockIcon ends up being an empty string and GetSpec thinks this is not a stock icon and returns a different (and malformed) spec.

There are 2 possible fixes here:
1. consider the uri valid, and rather use SetIsVoid on mStockIcon
2. consider the uri invalid and throw on creation

cc-ing Joe and Neil that recently touched this code, to have a clue what's the proper approach here. I can make a patch and fix the test once this has a direction.
Comment 7 neil@parkwaycc.co.uk 2011-03-05 15:31:30 PST
moz-icon://stock/ didn't work before Josh's rewrite either, it deserialises as moz-icon:////stock/. My preference is that this is an invalid URI.
Comment 8 Marco Bonardo [::mak] 2011-03-07 06:03:07 PST
Created attachment 517401 [details] [diff] [review]
patch v1.0

Yes, that sounds like the best option, the idl and comments are suggesting that a valid uri has a icon identifier too.
Comment 9 Marco Bonardo [::mak] 2011-03-07 06:05:13 PST
I don't think this has to block the release, but if we respin the rc would be nice to get it as a ride-along, this crash can be caused at will just adding a link to a page or visiting that uri through js.
Comment 10 Marco Bonardo [::mak] 2011-03-07 06:14:18 PST
Created attachment 517403 [details] [diff] [review]
patch v1.1

sorry, just a c/p typo in the test.
Comment 11 Dietrich Ayala (:dietrich) 2011-03-07 09:00:10 PST
(In reply to comment #9)
> I don't think this has to block the release, but if we respin the rc would be
> nice to get it as a ride-along, this crash can be caused at will just adding a
> link to a page or visiting that uri through js.

A few questions:

Any link or visit-via-js? Are there STR that reflect how a normal user would hit this?

Is this showing up in topcrashers?

Is there a risk of regression in other code that consumes this, now that it throws a new exception type in this case? (or is crash the only possible scenario there...)
Comment 12 Marco Bonardo [::mak] 2011-03-07 09:10:57 PST
(In reply to comment #11)
> Any link or visit-via-js? Are there STR that reflect how a normal user would
> hit this?

I'm mostly thinking to a DOS attack page here, basically anything that could cause a visit to the malformed uri would crash us.  I don't think a common user would hit this unless he hits such a malicious website.  A webdev could hit this during development.

> Is this showing up in topcrashers?

Not global ones, and I don't expect (hope) it to.

> Is there a risk of regression in other code that consumes this, now that it
> throws a new exception type in this case? (or is crash the only possible
> scenario there...)

Well, as Neil said above in comment 7, this specific uri never worked correctly, currently you can get a nsIURI out of it, but it's a nsIURI pointing to a broken/wrong/malformed uri, not even the same you built, so it is even worse.  All idls and documents specify the correct form of the uri is moz-icon://stock/<icon-identifier>.
Comment 13 Joe Drew (not getting mail) 2011-03-07 13:15:33 PST
Comment on attachment 517403 [details] [diff] [review]
patch v1.1

Switch with mStockIcon.IsEmpty() if it's possible.
Comment 14 Marco Bonardo [::mak] 2011-03-07 14:05:54 PST
(In reply to comment #13)
> Switch with mStockIcon.IsEmpty() if it's possible.

sure, will do.
Comment 15 Marco Bonardo [::mak] 2011-03-11 06:34:10 PST
Created attachment 518704 [details] [diff] [review]
patch v1.2

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