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
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
Last good nightly: 2010-12-15 First bad nightly: 2010-12-16 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66036625795f&tochange=f11f7ed625ba
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
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: 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.
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] 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.
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] patch v1.1 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] patch v1.1 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] patch v1.2