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]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Pascal Chevrel:pascalc]

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]

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]

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]]

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]]

taking because sounds like could be important

Comment 5

[Marco Bonardo [:mak]]

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]]

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

[[e-mail]]

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]]

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

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]]

Attachment: patch v1.1

sorry, just a c/p typo in the test.

Comment 11

[Dietrich Ayala (:dietrich)]

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

(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)]

Comment on attachment 517403 [details] [diff] [review]
patch v1.1

Switch with mStockIcon.IsEmpty() if it's possible.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> Switch with mStockIcon.IsEmpty() if it's possible.

sure, will do.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Comment 16

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/6ed8cbde3273

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6ed8cbde3273