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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
DOM
--
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: smaug, Assigned: Neil Deakin)

Tracking

(4 keywords)

Trunk
mozilla1.8.1
x86
All
crash, regression, topcrash, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

Currently #24 and #31 in 
http://talkback-public.mozilla.org/reports/firefox/FFTrunk/FFTrunk-topcrashers.html
Marking blocking1.8.1? although currently there seems to be crashes only in trunk.
Flags: blocking1.8.1?

Updated

11 years ago
Severity: normal → critical
Keywords: crash, regression
There are new crashes also in 1.8.1.
@ PL_DHashTableEnumerate  is currently #40

Comment 3

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

Comment 4

11 years ago
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.
Keywords: topcrash

Comment 5

11 years ago
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.
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).
Summary: Bug 352704 seems to have caused new crashes, [@ nsSubstring::IsDependentOn], [@ PL_DHashTableEnumerate] → Bug 352704 seems to have caused new crashes, [@ nsSubstring::IsDependentOn], [@ PL_DHashTableEnumerate] [@nsSubstring::Assign]
(Assignee)

Comment 7

11 years ago
Created attachment 239351 [details] [diff] [review]
Guess at a fix
Assignee: general → enndeakin
Status: NEW → ASSIGNED
Attachment #239351 - Flags: superreview?(jst)
Attachment #239351 - Flags: review?(jst)

Comment 8

11 years ago
Neil - can we get your guess on the trunk asap to figure out whether it helps or not...
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 9

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

Sure

(Assignee)

Comment 10

11 years ago
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.
Attachment #239351 - Flags: superreview?(jst)
Attachment #239351 - Flags: review?(jst)
(Assignee)

Comment 11

11 years ago
Created attachment 239390 [details] [diff] [review]
OK, that was easier than I thought
Attachment #239351 - Attachment is obsolete: true
Attachment #239390 - Flags: superreview?(jst)
Attachment #239390 - Flags: review?(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
Attachment #239390 - Flags: superreview?(jst)
Attachment #239390 - Flags: superreview+
Attachment #239390 - Flags: review?(jst)
Attachment #239390 - Flags: review+

Comment 13

11 years ago
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?
> 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

11 years ago
(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.
> Hourly's TB says/send Nightly's BuildID

Ah, interesting.  OK.  Didn't know that.
Not "interesting".  Bug.  Bug 353747, to be exact.
(Assignee)

Comment 18

11 years ago
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.
TB23619803H - I tried remove all cookies -> Firefox crashes. 
2th test -> OK, all cookies removed.
(Assignee)

Comment 20

11 years ago
Created attachment 239759 [details] [diff] [review]
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
Attachment #239759 - Flags: superreview?(jst)
Attachment #239759 - Flags: review?(jst)
(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 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
Attachment #239759 - Flags: superreview?(jst)
Attachment #239759 - Flags: superreview+
Attachment #239759 - Flags: review?(jst)
Attachment #239759 - Flags: review+
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.
(Assignee)

Comment 24

11 years ago
Checked in the latest patch

Comment 25

11 years ago
(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).
So is this going to be in RC2?

Comment 27

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

11 years ago
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.
> 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

11 years ago
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.
> 

Created attachment 240027 [details] [diff] [review]
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.
Attachment #240027 - Flags: approval1.8.1?

Comment 32

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

Approved for RC2 - thanks!
Attachment #240027 - Flags: approval1.8.1? → approval1.8.1+
Neil, let me know if you'd like me to check this in for you.
(Assignee)

Comment 34

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

Sure, thanks.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Fixed on branch.
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1

Comment 36

11 years ago
*** Bug 354368 has been marked as a duplicate of this bug. ***
Verified on trunk based on trunk talkback:
http://talkback-public.mozilla.org/reports/firefox/FFTrunk/FFTrunk-topcrashers.html
Status: RESOLVED → VERIFIED

Comment 38

11 years ago
*** Bug 354448 has been marked as a duplicate of this bug. ***
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
Keywords: fixed1.8.1 → verified1.8.1
*** Bug 355781 has been marked as a duplicate of this bug. ***
*** Bug 355781 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Flags: in-testsuite?
Crash Signature: [@ nsSubstring::IsDependentOn] [@ PL_DHashTableEnumerate] [@nsSubstring::Assign]
You need to log in before you can comment on or make changes to this bug.