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
Fx 3.6.13 does not crashes
Time 2010-12-22 11:32:34.425479
Last Crash 74 seconds before submission
Install Age 28736 seconds (8.0 hours) since version was first installed.
Build ID 20101222030351
OS Version 0.0.0 Linux 2.6.35-24-generic #42-Ubuntu SMP Thu Dec 2 01:41:57 UTC 2010 i686
CPU Info GenuineIntel family 6 model 23 stepping 6
Crash Reason SIGSEGV
Crash Address 0x0
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
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
Last good nightly: 2010-12-15 First bad nightly: 2010-12-16
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.
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
taking because sounds like could be important
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.
So, nsMozIconURI acts wrongly on this specific URI, the correct form is:
while in this case we have:
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.
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.
Created attachment 517401 [details] [diff] [review]
Yes, that sounds like the best option, the idl and comments are suggesting that a valid uri has a icon identifier too.
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.
Created attachment 517403 [details] [diff] [review]
sorry, just a c/p typo in the test.
(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...)
(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 on attachment 517403 [details] [diff] [review]
Switch with mStockIcon.IsEmpty() if it's possible.
(In reply to comment #13)
> Switch with mStockIcon.IsEmpty() if it's possible.
sure, will do.
Created attachment 518704 [details] [diff] [review]