353227 - Bug 352704 seems to have caused new crashes, [@ nsSubstring::IsDependentOn], [@ PL_DHashTableEnumerate] [@nsSubstring::Assign]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently #24 and #31 in 
http://talkback-public.mozilla.org/reports/firefox/FFTrunk/FFTrunk-topcrashers.html

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Marking blocking1.8.1? although currently there seems to be crashes only in trunk.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

There are new crashes also in 1.8.1.
@ PL_DHashTableEnumerate  is currently #40

Comment 3

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060919 BonEcho/2.0 ID:2006091905

TB[@ PL_DHashTableEnumerate] (23481585 is mine) http://tinyurl.com/kv9n4
will this be fixed before official Firefox 2 RC1 ?

Comment 4

[Neil Deakin]

Hopefully, but I've tried various things and can't get it to crash. Here's what I've tried:
 - with no storage data
 - with some session data only
 - with global and/or session data
 - with and without a db file present
 - with a readonly db
 - with cookies present and not present
 - both Mac and Windows
 - choosing to Clear Private Data from the menu
 - choosing Remove All from the cookie dialog
 - choosing Clear Now from the Privacy tab
 - setting the option to clear on exit
 - turning on and off the ask me being clearing on exit option
 - clear automatically when a restart is needed due to an extension install

The crashes seem to always be when enumerating the hashtable of a particular DOM storage object, yet such an object shouldn't exist unless DOM storage has been used during the session.

PL_DHashTableEnumerate  [mozilla\xpcom\build\pldhash.c, line 682]
nsTHashtable<nsUniCharEntry>::EnumerateEntries  [mozilla\dist\include\xpcom\nsthashtable.h, line 238]
nsDOMStorage::ClearAll  [mozilla\dom\src\storage\nsdomstorage.cpp, line 697]
nsTHashtable<nsUniCharEntry>::EnumerateEntries  [mozilla\dist\include\xpcom\nsthashtable.h, line 238]
nsDOMStorageManager::Observe  [mozilla\dom\src\storage\nsdomstorage.cpp, line 160]
...

It always seems to get past the first EnumerateEntries which should be an empty hashtable also.

Comment 5

[pal-moz]

clearly, almost of crashes like this happens after check-in for bug 352704.
so, I think, the check-in for bug352704 should be backed-out or this bug should be fixed ASAP, before RC1 or final.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is showing up as a topcrash on the 1.8 branch under the signatures PL_DHashTableEnumerate (same as trunk) and nsSubstring::Assign (different from trunk).

Comment 7

[Neil Deakin]

Attachment: Guess at a fix

Comment 8

[Mike Schroepfer]

Neil - can we get your guess on the trunk asap to figure out whether it helps or not...

Comment 9

[Neil Deakin]

(In reply to comment #8)
> Neil - can we get your guess on the trunk asap to figure out whether it helps
> or not...
> 

Sure

Comment 10

[Neil Deakin]

Comment on attachment 239351 [details] [diff] [review]
Guess at a fix

If this is the cause of the bug, I'll make another patch which releases as well.

Comment 11

[Neil Deakin]

Attachment: OK, that was easier than I thought

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 239390 [details] [diff] [review]
OK, that was easier than I thought

This is for sure the right thing to do, my only comment is:

- In nsDOMStorageManager::Initialize():

   gStorageManager = new nsDOMStorageManager();
   if (!gStorageManager)
     return NS_ERROR_OUT_OF_MEMORY;
 
-  if (!gStorageManager->mStorages.Init())
+  if (!gStorageManager->mStorages.Init()) {
+    delete gStorageManager;
+    gStorageManager = nsnull;
     return NS_ERROR_OUT_OF_MEMORY;
+  }
+
+  NS_ADDREF(gStorageManager);

I don't feel very strongly about this, but I'd rather see this addref done right after the new new and oom check and then simply release gStorageManager if initialization fails (NS_RELEASE() would null out the pointer for you too). A bit less hand-writing of memory management code and more usage of existing mechanisms for dealing with things like this.

Either way, r+sr=jst

Comment 13

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060921 Minefield/3.0a1 ID:2006092118 [cairo]

check-in 2006-09-21 06:55,
but seems to be not fixed.
TB23593871Z [@ PL_DHashTableEnumerate]
TB23593011H [@ nsSubstring::ReplacePrep] / anohter bug?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> check-in 2006-09-21 06:55,

And your talkback incidents say:

  Build ID  2006092104

Which would be about 3 hours before the checkin.

Try a newer build?

Comment 15

[pal-moz]

(In reply to comment #14)
> Try a newer build?

already using Hourly, see UA in comment#13 carefully.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060921 Minefield/3.0a1 ID:2006092118 [cairo]

Hourly's TB says/send Nightly's BuildID, I don't know why.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Hourly's TB says/send Nightly's BuildID

Ah, interesting.  OK.  Didn't know that.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not "interesting".  Bug.  Bug 353747, to be exact.

Comment 18

[Neil Deakin]

The possible crash fix was actually the first patch, checked in on 2006-09-20 10:36. So it looks like it didn't fix the crash.

Comment 19

[Pavel Cvrcek [:JasnaPaka]]

TB23619803H - I tried remove all cookies -> Firefox crashes. 
2th test -> OK, all cookies removed.

Comment 20

[Neil Deakin]

Attachment: Remove the right objects from the hash

So why did this even compile before?

Anyway this fixes a crash with the right stack trace, but it would only occur if DOM storage had been used. Wouldn't think that would be the case.

Anyway, the steps I used to reproduce:
1. Go to http://xulplanet.com/ndeakin/tests/sessions/crasho.html
2. Press the button
3. Close the tab and open a new tab and load the same page again and press the button.
4. Select Clear Private Data
  -> crash

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #20)
> So why did this even compile before?

Because the hash entry type is derived from nsVoidPtrHashKey and doesn't override much, so the method takes void*.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 239759 [details] [diff] [review]
Remove the right objects from the hash

 nsDOMStorageManager::RemoveFromStoragesHash(nsDOMStorage* aStorage)
 {
  nsDOMStorageEntry* entry = mStorages.GetEntry(aStorage);
   if (entry)
-    mStorages.RemoveEntry(entry);
+    mStorages.RemoveEntry(aStorage);
 }

Don't you want to use RawRemoveEntry() here? That should take the entry you've already looked up and remove w/o doing the lookup again. That'd only be a performance difference, so either way is cool with me for now to get this in to see if the crash goes away.

r+sr=jst

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

RawRemoveEntry has the disadvantage of never shrinking the table (a bigger deal if this is the only thing that ever removes entries).

Note that RawRemoveEntry takes an entry as an argument and RemoveEntry takes a key as an argument -- a somewhat confusing API, IMO.

Comment 24

[Neil Deakin]

Checked in the latest patch

Comment 25

[pal-moz]

(In reply to comment #24)
> Checked in the latest patch
> 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060923 Minefield/3.0a1 ID:2006092319 [cairo]

seems to be fixed for me.

and if this is fixed by latest patch, if possible, new RC1 candidate build should be made.
because this bug exist on RC1 candidate build (2006091818).

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

So is this going to be in RC2?

Comment 27

[jigar shah]

Yes...Its very easily reproducible....on
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/2006091817 Firefox/2.0

Comment 28

[nightisfalling]

RC2?

If you remember this post by Mike Connor...

REMINDER: 1.8.1/Fx2 RC1 freeze is tonight

For anyone not following the developer calendar, freeze is tonight.  We’re really in the endgame, with a small set of bugs we’re aggressively driving to completion.
The intention is for this to be a true Release Candidate, meaning that if we do not find any new bugs, RC1 will be shipped as Firefox 2.  If there are things that need to be on our radar, please nominate them now.  After RC1, we will not be taking patches for problems we knew about and shipped in RC1.

If you are not sure about a potential blocker, please find schrep, beltzner, dbaron, darin and myself to discuss it ASAP. 

If this is left out of RC1, so much for a true RC...yet again.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> If you are not sure about a potential blocker, please find schrep, beltzner,
> dbaron, darin and myself to discuss it ASAP. 

Did you do this?

I should note that this bug is currently marked as blocking RC1, for what it's worth.

Comment 30

[Mike Schroepfer]

So this bug was filed *after* the RC1 codefreeze.  So seems to be that fits exactly what Mike is talking about.   I.e. new information causing us to want to spin a new build of higher quality.  

(In reply to comment #28)
> RC2?
> 
> If you remember this post by Mike Connor...
> 
> REMINDER: 1.8.1/Fx2 RC1 freeze is tonight
> 
> For anyone not following the developer calendar, freeze is tonight.  We’re
> really in the endgame, with a small set of bugs we’re aggressively driving to
> completion.
> The intention is for this to be a true Release Candidate, meaning that if we do
> not find any new bugs, RC1 will be shipped as Firefox 2.  If there are things
> that need to be on our radar, please nominate them now.  After RC1, we will not
> be taking patches for problems we knew about and shipped in RC1.
> 
> If you are not sure about a potential blocker, please find schrep, beltzner,
> dbaron, darin and myself to discuss it ASAP. 
> 
> If this is left out of RC1, so much for a true RC...yet again.
> 

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Cumulative 1.8 branch patch

This includes attachment 239390 [details] [diff] [review] and attachment 239759 [details] [diff] [review], and is diffed against the 1.8 branch.

Nominating per discussion with schrep.

Comment 32

[Mike Schroepfer]

Comment on attachment 240027 [details] [diff] [review]
Cumulative 1.8 branch patch

Approved for RC2 - thanks!

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Neil, let me know if you'd like me to check this in for you.

Comment 34

[Neil Deakin]

(In reply to comment #33)
> Neil, let me know if you'd like me to check this in for you.
> 

Sure, thanks.

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on branch.

Comment 36

[Adam Guthrie]

*** Bug 354368 has been marked as a duplicate of this bug. ***

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Verified on trunk based on trunk talkback:
http://talkback-public.mozilla.org/reports/firefox/FFTrunk/FFTrunk-topcrashers.html

Comment 38

[Adam Guthrie]

*** Bug 354448 has been marked as a duplicate of this bug. ***

Comment 39

[Tracy Walker [:tracy]]

verified on Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060928 BonEcho/2.0 with steps in comment 20

Comment 40

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 355781 has been marked as a duplicate of this bug. ***

Comment 41

[Dave Townsend [:mossop]]

*** Bug 355781 has been marked as a duplicate of this bug. ***