Closed Bug 548685 Opened 14 years ago Closed 8 years ago

null dereference crash in nsURIHashKey::HashKey

Categories

(Core :: Networking, defect)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: dbaron, Assigned: valentin)

References

Details

(Keywords: crash, Whiteboard: [tbird crash][necko-active])

Crash Data

Attachments

(1 file, 1 obsolete file)

There's a new trunk topcrash in today's builds:

http://crash-stats.mozilla.com/report/list?product=Firefox&branch=1.9.3&platform=windows&query_search=signature&query_type=exact&query=&date=&range_value=31&range_unit=days&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=nsTHashtable%3CnsBaseHashtableET%3CnsURIHashKey%2C%20nsCOMPtr%3CnsIXBLDocumentInfo%3E%20%3E%20%3E%3A%3As_HashKey%28PLDHashTable*%2C%20void%20const*%29

Top of stack is:
0  	xul.dll  	nsTHashtable<nsBaseHashtableET<nsURIHashKey,nsCOMPtr<nsIXBLDocumentInfo> > >::s_HashKey  	 obj-firefox/dist/include/nsTHashtable.h:365
1 	xul.dll 	PL_DHashTableOperate 	obj-firefox/xpcom/build/pldhash.c:615
2 	xul.dll 	mozilla::places::History::UnregisterVisitedCallback 	toolkit/components/places/src/History.cpp:283
3 	xul.dll 	mozilla::dom::Link::UnregisterFromHistory 	content/base/src/Link.cpp:518
4 	xul.dll 	nsHTMLAnchorElement::`vector deleting destructor' 	
5 	xul.dll 	nsNodeUtils::LastRelease 	content/base/src/nsNodeUtils.cpp:274


I presume it's a regression from bug 461199.
Summary: topcrash [@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIXBLDocumentInfo> > >::s_HashKey(PLDHashTable*, void const*) ] → null dereference topcrash [@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIXBLDocumentInfo> > >::s_HashKey(PLDHashTable*, void const*) ]
My guess is that if this were a DEBUG build, this assertion in nsLink.cpp:
   514  NS_ASSERTION(mCachedURI, "mRegistered is true, but we have no cached URI?!");
would be failing.  It's not clear to me what guarantees that that's true.
(In reply to comment #1)
> My guess is that if this were a DEBUG build, this assertion in nsLink.cpp:
>    514  NS_ASSERTION(mCachedURI, "mRegistered is true, but we have no cached
> URI?!");
> would be failing.  It's not clear to me what guarantees that that's true.
We should never, ever, be registering if we do not have a cached URI.  Do we have steps to reproduce?
Can we add a runtimeabort at that assertion to test the theory? I tried to load the minidumps to check the parameter values but that seems to be broken today.

There aren't that many reports here, although there is more than one user crashing, but the reports all have many (20+) extensions installed; I don't think this needs to be an alpha blocker.
Is this the same as the #2 topcrash from SeaMonkey 2.1a1pre?

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=nsTHashtable%3Cmozilla%3A%3Aplaces%3A%3AHistory%3A%3AKeyClass%3E%3A%3As_HashKey%28PLDHashTable%2A%2C%20void%20const%2A%29&version=SeaMonkey%3A2.1a1pre

This one appeared in the 2010-02-18 nightlies, where bug 461199 was in, then disappeared when it got backed out, and re-appeared again starting with 2010-02-25 nightlies when that one was in again, so looks like a tight correlation.
This is probably the same as bug 546900, which has steps to repro.
Depends on: 546900
Per bug 546900 comment 13, I don't think this is the same bug.
No longer depends on: 546900
I think this is now showing up as alternating between these two signatures:

[@ nsURIHashKey::HashKey(nsIURI const*) ]
[@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] 

but otherwise the same stack.

The users hitting this crash all seem to have very large numbers of extensions installed (maybe just one user?).  It's likely a similar problem to bug 546900, but in an nsIURI implementation in an extension.
Summary: null dereference topcrash [@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIXBLDocumentInfo> > >::s_HashKey(PLDHashTable*, void const*) ] → null dereference topcrash [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ]
I reported several crashes today, one of them with a new profile:
http://crash-stats.mozilla.com/report/index/5f00b4c0-6e9b-4858-a998-0f89e2101010

I can reproduce it always with imap and pop3 accounts.

1) read a message
2) move to a message with vcf-card attatchment
3) move to any other message
4) move to a message with vcf-card attatchment
5) move to ...CRASH
Crash Signature: [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ]
Crash Signature: [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] → void const*)] [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] [@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIStorageStream> > >::s_HashKey(PLDHashTable*
This currently seems to be happening quite a bit on 7.0 (#43), and some on 9.0a1.
Crash Signature: void const*)] → void const*)] [@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ]
Why does this bug not come on the crash report here:

https://crash-stats.mozilla.com/report/index/bp-2d08d6bb-4966-406f-8cf5-df7962110902

[@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIStorageStream> > >::s_HashKey(PLDHashTable*, void const*) ]
Perhaps it needs a space before the "]".  I'm adding it...
Crash Signature: void const*)] [@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ] → void const*) ] [@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ]
Depends on: 677643
Bug 677643 has steps to reproduce the crash.
I was not able to reproduce this using the above websites.

I have a faster STR or more accurate (for sure crash on Linux/WinXP/Win7) STR you can say:
0. Remember to keep the Flash plugin enabled for this STR.
1. Install HTTPS Everywhere.
2. Install HTTPS Finder (HTTPS Finder 0.75) @ https://code.google.com/p/https-finder/downloads/list
3. Browse to http://ihaveaplan.ca and http://www.ihaveaplan.ca
4. You should get prompt to save new HTTPS rules for each of the site above.
5. Save the 2 rules and restart.
6. Go to ihaveaplan.ca and under "List of student associations" click "UFV - University of the Fraser Valley (SUS)"
7. On the third box in the middle (under the video) it says "HOW MUCH DOES 
IT COST?" and there is hyperlink to "Your annual fee is $159.92 for coverage including health, dental, vision, and travel. ". 
8. Click the above hyperlink and it should take you to a 404 site (https) if you followed the above steps properly. Wait a few seconds (there is something on that page that tries to redirect you to the home page automatically in a few seconds) and Firefox will crash.

If you did NOT follow the steps properly it will take you here: http://ihaveaplan.ca/rte/en/UniversityCollegeoftheFraserValleySUS_Health_HowMuchDoesItCost (which does not crash)
The Crash URL, last URL before it crashes firefox, is = https://www.ihaveaplan.ca/UniversityCollegeoftheFraserValleySUS_Health_HowMuchDoesItCost
(In reply to Mats Palmgren [:mats] from comment #14)
> Perhaps it needs a space before the "]".  I'm adding it...



Still doesn't show up on Crash Stats:

https://crash-stats.mozilla.com/report/index/bp-3d4bf503-9e92-46b8-b51a-f7b782110903

[@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIStorageStream> > >::s_HashKey(PLDHashTable*, void const*) ] 

I think Socorro associates 1 bug with 1 to 2 signatures. I don't think Socorro is able to associate when there are more signatures.
The weird thing is if you go directly to (crash url) https://www.ihaveaplan.ca/UniversityCollegeoftheFraserValleySUS_Health_HowMuchDoesItCost without following the exact STR in Comment 16 after approx 15 seconds you are redirected (without crashing) to https://www.ihaveaplan.ca/
(In reply to Vic from comment #13)
> Why does this bug not come on the crash report here

The crash-stats system seem to have some peculiarities wrt to showing bug numbers, I'm working with them to get that fixed, but that discussion doesn't belong in this bug but in bug 681682 or something connected to it.
For the first siganture - most of these are appearing on current versions of Thunderbird. About 615 of these on Thunderbird 8.0 in the past 4 weeks = #48.

For the second signature -nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) - all are on FF in reasonable volume ie:886 on 7.0.1 in 4 weeks - #118.

Leaving the top crash keyword since it's much higher for Thunderbird.
> For the second signature
> -nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*,
> void const*) - all are on FF in reasonable volume ie:886 on 7.0.1 in 4 weeks
> - #118.

bug 677643 is fixed from 8.0 on
Severity: normal → critical
https://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=nsURIHashKey%3A%3AHashKey(nsIURI%20const*) says that almost all of the crashes in the last 4 weeks were from Thunderbird or SeaMonkey, but we had a few on Firefox 8 beta.
nsURIHashKey::HashKey (the Mac/Linux variant) has a couple on Firefox desktop and even Fennec, but also few.
Crash Signature: void const*) ] [@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ] [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] [@ nsTHashtable<… → nsCOMPtr<nsIStorageStream> > >::s_HashKey(PLDHashTable*, void const*) ] [@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ] [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsURIHashKey::HashKey ] [@ nsTHashtable<mozilla::…
Comment #3 and comment #7 mention a large number of extensions (e.g., 20+) as contributing to this problem.  I encountered this problem while switching from profile with 15 extensions to a profile with 17 extensions.  

Comment #16 refers to a 404 Web site.  Other comments mention external Web pages.  I encountered this problem while switching from one profile to another profile where both profiles have home pages that are local HTML files on my hard drive.  The HTML files are the exported bookmarks for each profile.  No access to any external Web page should have been involved with the crash.
Also note well:  This bug report cites Windows 7 as the platform OS.  I encountered this crash with Windows XP SP3.
Summary: null dereference topcrash [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ] → null dereference crash in mozilla::places::History::UnregisterVisitedCallback
I see only 1 of these crashes in FF in the past week. None in SeaMonkey and none in Thunderbird - unless my search is wrong. Seems like it's no longer a top crash?
Thos two signatures seem to be around in recent builds as well, but not too high:

[@ nsCacheEntryHashTable::HashKey(PLDHashTable*, void const*) ]
[@ nsURIHashKey::HashKey(nsIURI const*) ]

Those are around with somewhat higher frequency - see also https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&date=02%2F14%2F2012+10%3A34%3A02&query_search=signature&query_type=contains&query=%3A%3As_HashKey&reason=&build_id=&process_type=any&hang_type=any&do_query=1 - but that might not count as topcrashes:

[@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsRefPtr<nsCSSStyleSheet> > >::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsIdentifierMapEntry>::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIStorageStream> > >::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, CacheScriptEntry> >::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsBaseHashtableET<nsStringHashKey, EventNameMapping> >::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsCOMPtr<nsIURI> > >::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ]
[@ nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsCOMPtr<nsISupports> > >::s_HashKey(PLDHashTable*, void const*) ]
[@ nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsIAtom*> >::s_HashKey(PLDHashTable*, void const*) ]
Crash Signature: nsCOMPtr<nsIStorageStream> > >::s_HashKey(PLDHashTable*, void const*) ] [@ nsTHashtable<nsNavHistory::VisitHashKey>::s_HashKey(PLDHashTable*, void const*) ] [@ nsURIHashKey::HashKey(nsIURI const*) ] [@ nsURIHashKey::HashKey ] [@ nsTHashtable<mozilla::… → nsIAtom*> >::s_HashKey(PLDHashTable*, void const*) ] void const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey ] [@ nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsCOMPtr<nsISupports> > >::s_HashKey(PLDHashTable*, void const*) ] […
Whiteboard: [tbird topcrash]
This seems to be much higher on Thunderbird but not as high on desktop. I am removing the keyword but it's still tagged as a Thunderbird topcrash.
Keywords: topcrash
Blocks: 760056
#2 crash for Thunderbird 10.0.6esr
but not a topcrash for TB14 - crash rate for TB14 substantially lower than TB12
Whiteboard: [tbird topcrash] → [tbird crash]
Crash Signature: , void const*) ] [@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsRefPtr<nsCSSStyleSheet> > >::s_HashKey(PLDHashTable*, void const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, nsIAtom*> >::s_HashKey(PLDHashTable*, … → , void const*) ] [@ nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsRefPtr<nsCSSStyleSheet> > >::s_HashKey(PLDHashTable*, void const*) ] [@ nsTHashtable<mozilla::places::History::KeyClass>::s_HashKey(PLDHashTable*, nsIAtom*> >::s_HashKey(PLDHashTable*, v…
Assignee: nobody → valentin.gosu
Component: History: Global → Networking
Summary: null dereference crash in mozilla::places::History::UnregisterVisitedCallback → null dereference crash in nsURIHashKey::HashKey
MozReview-Commit-ID: 5wCZ0DTHEUS
Attachment #8742385 - Flags: review?(mcmanus)
Comment on attachment 8742385 [details] [diff] [review]
Avoid null pointer deref in nsURIHashKey

Review of attachment 8742385 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsURIHashKey.h
@@ +29,5 @@
>  
>      bool KeyEquals(const nsIURI* aKey) const {
>          bool eq;
> +        if (!mKey && !aKey) {
> +            return true;

I think we ought to handle this case too.
Found a few crashes for it, such as: https://crash-stats.mozilla.com/report/index/c3c35e0f-cef6-438d-82f9-8f0a72160415

@@ +42,5 @@
>      static PLDHashNumber HashKey(const nsIURI* aKey) {
>          nsAutoCString spec;
> +        if (!aKey) {
> +            // If the key is null, return hash for empty string.
> +            return mozilla::HashString(spec);

We could return 0 or another magic number instead here.
(In reply to Valentin Gosu [:valentin] from comment #35)
> > >      bool KeyEquals(const nsIURI* aKey) const {
> >          bool eq;
> > +        if (!mKey && !aKey) {
> > +            return true;

And this should actually be:
> if (!mKey) return !aKey;
Comment on attachment 8742385 [details] [diff] [review]
Avoid null pointer deref in nsURIHashKey

Review of attachment 8742385 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for chasing crashes.

::: netwerk/base/nsURIHashKey.h
@@ +29,5 @@
>  
>      bool KeyEquals(const nsIURI* aKey) const {
>          bool eq;
> +        if (!mKey && !aKey) {
> +            return true;

in hashkey() null is treated as empty and here it is treated as "true".. I think you should treat it as empty here too. (i.e. aKey==nullptr and mKey = "foo" shouldn't match.. but mkey = "" and akey=null should)

@@ +42,5 @@
>      static PLDHashNumber HashKey(const nsIURI* aKey) {
>          nsAutoCString spec;
> +        if (!aKey) {
> +            // If the key is null, return hash for empty string.
> +            return mozilla::HashString(spec);

I think you can use EmptyCString
Attachment #8742385 - Flags: review?(mcmanus) → review-
MozReview-Commit-ID: 5wCZ0DTHEUS
Attachment #8742408 - Flags: review?(mcmanus)
Attachment #8742385 - Attachment is obsolete: true
(In reply to Patrick McManus [:mcmanus] from comment #37)
> Comment on attachment 8742385 [details] [diff] [review]
> Avoid null pointer deref in nsURIHashKey
> 
> Review of attachment 8742385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thanks for chasing crashes.
> 
> ::: netwerk/base/nsURIHashKey.h
> @@ +29,5 @@
> >  
> >      bool KeyEquals(const nsIURI* aKey) const {
> >          bool eq;
> > +        if (!mKey && !aKey) {
> > +            return true;
> 
> in hashkey() null is treated as empty and here it is treated as "true".. I
> think you should treat it as empty here too. (i.e. aKey==nullptr and mKey =
> "foo" shouldn't match.. but mkey = "" and akey=null should)

I wrote it this way because a null URI isn't equal to a URI with an empty spec. Even though they happen to have the same hash in this case, I think that's OK because a URI with an empty spec isn't very common.
Even now, if mKey is not null, mKey->Equals(null) will return false.
Whiteboard: [tbird crash] → [tbird crash][necko-active]
(In reply to Valentin Gosu [:valentin] from comment #39)
> (In reply to Patrick McManus [:mcmanus] from comment #37)
> > Comment on attachment 8742385 [details] [diff] [review]
> > Avoid null pointer deref in nsURIHashKey
> > 
> > Review of attachment 8742385 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > thanks for chasing crashes.
> > 
> > ::: netwerk/base/nsURIHashKey.h
> > @@ +29,5 @@
> > >  
> > >      bool KeyEquals(const nsIURI* aKey) const {
> > >          bool eq;
> > > +        if (!mKey && !aKey) {
> > > +            return true;
> > 
> > in hashkey() null is treated as empty and here it is treated as "true".. I
> > think you should treat it as empty here too. (i.e. aKey==nullptr and mKey =
> > "foo" shouldn't match.. but mkey = "" and akey=null should)
> 
> I wrote it this way because a null URI isn't equal to a URI with an empty
> spec. Even though they happen to have the same hash in this case, I think
> that's OK because a URI with an empty spec isn't very common.
> Even now, if mKey is not null, mKey->Equals(null) will return false.

I guess that's ok.. but then null compared !=null should return false, right?
Comment on attachment 8742408 [details] [diff] [review]
Avoid null pointer deref in nsURIHashKey

this patch is marked both obsolete and r?.. not sure what to do.. just clearing the review flag without prejudice :)
Attachment #8742408 - Flags: review?(mcmanus)
Comment on attachment 8742408 [details] [diff] [review]
Avoid null pointer deref in nsURIHashKey

scratch that.. r? back on - confused by prior comment
Attachment #8742408 - Flags: review?(mcmanus)
Attachment #8742408 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/0c83c41da3fd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8742408 [details] [diff] [review]
Avoid null pointer deref in nsURIHashKey

Approval Request Comment

[Feature/regressing bug #]:
Always present.

[User impact if declined]:
A fair number of crashes.

[Describe test coverage new/current, TreeHerder]:
None.

[Risks and why]:
Very low. Patch only adds a check to prevent null pointer deref.

[String/UUID change made/needed]:
None.
Attachment #8742408 - Flags: approval-mozilla-beta?
Comment on attachment 8742408 [details] [diff] [review]
Avoid null pointer deref in nsURIHashKey

Crash fix, Beta47+
Attachment #8742408 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.