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]

RESOLVED FIXED in mozilla5

Status

()

Toolkit
Places
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pascalc, Assigned: mak)

Tracking

({crash, regression})

Trunk
mozilla5
x86
All
crash, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [places-next-wanted], crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
Component: General → Bookmarks & History
QA Contact: general → bookmarks
Summary: Crash with with moz-icon:// url → Crash with with moz-icon:// url [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ]

Updated

6 years ago
Severity: normal → critical
(Assignee)

Updated

6 years ago
Blocks: 560198
(Reporter)

Comment 2

6 years ago
Last good nightly: 2010-12-15 First bad nightly: 2010-12-16

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66036625795f&tochange=f11f7ed625ba

Updated

6 years ago
Component: Bookmarks & History → History: Global
OS: Linux → All
Product: Firefox → Core
QA Contact: bookmarks → history.global
Summary: Crash with with moz-icon:// url [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ] → Crash with with moz-icon:// url [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ][@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ]
Version: unspecified → Trunk

Updated

6 years ago
Component: History: Global → Places
Product: Core → Toolkit
QA Contact: history.global → places
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Comment 4

6 years ago
taking because sounds like could be important
Assignee: nobody → mak77
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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.
Attachment #517401 - Flags: review?(joe)
(Assignee)

Comment 9

6 years ago
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.
blocking2.0: --- → ?
(Assignee)

Comment 10

6 years ago
Created attachment 517403 [details] [diff] [review]
patch v1.1

sorry, just a c/p typo in the test.
Attachment #517401 - Attachment is obsolete: true
Attachment #517403 - Flags: review?(joe)
Attachment #517401 - Flags: review?(joe)
(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...)
(Assignee)

Updated

6 years ago
Summary: Crash with with moz-icon:// url [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ][@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] → 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]
(Assignee)

Comment 12

6 years ago
(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>.

Updated

6 years ago
blocking2.0: ? → -
Comment on attachment 517403 [details] [diff] [review]
patch v1.1

Switch with mStockIcon.IsEmpty() if it's possible.
Attachment #517403 - Flags: review?(joe) → review+
(Assignee)

Comment 14

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

sure, will do.
(Assignee)

Comment 15

6 years ago
Created attachment 518704 [details] [diff] [review]
patch v1.2
Attachment #517403 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [places-next-wanted]
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/6ed8cbde3273
Keywords: checkin-needed
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-cedar]
Target Milestone: --- → mozilla2.2
http://hg.mozilla.org/mozilla-central/rev/6ed8cbde3273
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [places-next-wanted][fixed-in-cedar] → [places-next-wanted]
Crash Signature: [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] [@ mozilla::places::URIBinder::Bind]
You need to log in before you can comment on or make changes to this bug.