Last Comment Bug 614499 - Crash [@ nsSHTransaction::GetPrev ]
: Crash [@ nsSHTransaction::GetPrev ]
Status: RESOLVED FIXED
: [sg:critical] testcase doesn't affect...
: crash, testcase
Product: Core
Classification: Components
Component: Document Navigation
: Trunk
: x86 Windows 7
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: docshell
:
:
: 326633 594645 612881 612887
  Show dependency treegraph
 
Reported: 2010-11-24 00:16 PST by Jesse Ruderman
Modified: 2011-06-20 18:08 PDT (History)
10 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
[@ nsSHTransaction::GetPrev ]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  final+
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
testcase (crashes Firefox when loaded) (341 bytes, text/html)
2010-11-24 00:16 PST, Jesse Ruderman
no flags Details
stack trace (12.57 KB, text/plain)
2010-11-24 00:19 PST, Jesse Ruderman
no flags Details
patch, clear the pointers when modifying mListRoot (888 bytes, patch)
2010-11-24 05:59 PST, Olli Pettay [:smaug]
bzbarsky: review+
clegnitto: approval1.9.2.14-
clegnitto: approval1.9.1.17-
Details | Diff | Splinter Review
null check aNext (1.62 KB, patch)
2011-01-04 04:20 PST, Olli Pettay [:smaug]
bzbarsky: review+
clegnitto: approval1.9.2.14+
clegnitto: approval1.9.1.17+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2010-11-24 00:16:23 PST
Created attachment 492931 [details]
testcase (crashes Firefox when loaded)

This might be the same bug as topcrash bug 612881 / bug 612887.
Comment 1 Jesse Ruderman 2010-11-24 00:19:29 PST
Created attachment 492932 [details]
stack trace
Comment 2 Jesse Ruderman 2010-11-24 00:25:27 PST
The top frame of the stack is random, so this could be anywhere between topcrash #1 and topcrash #11.  Fixing it in beta8 would be nice.
Comment 3 Justin Lebar [:jlebar] 2010-11-24 00:28:59 PST
Is this win7 only?  It doesn't crash my Linux x64 box.

Will definitely look at this tomorrow.
Comment 4 Jesse Ruderman 2010-11-24 01:29:39 PST
Debug builds crash on Windows, Mac, and Linux.  Opt builds crash on at least Windows and Mac.
Comment 5 Olli Pettay [:smaug] 2010-11-24 03:01:18 PST
Justin, I took this before noticing your comment.
I assume this might be a regression from my changes.
Comment 6 Olli Pettay [:smaug] 2010-11-24 05:56:45 PST
Ah, the code causing this has been there for ages, at least from year 2001.
Comment 7 Olli Pettay [:smaug] 2010-11-24 05:59:46 PST
Created attachment 492984 [details] [review]
patch, clear the pointers when modifying mListRoot
Comment 8 Olli Pettay [:smaug] 2010-11-24 06:03:06 PST
Paul, does session restore get nsISHTransaction.prev anywhere in the branches?
C++ does seem to call ::GetPrev in 1.9.1 nor 1.9.2.
Comment 9 Paul O'Shannessy [:zpao] 2010-11-24 09:16:38 PST
(In reply to comment #8)
> Paul, does session restore get nsISHTransaction.prev anywhere in the branches?
> C++ does seem to call ::GetPrev in 1.9.1 nor 1.9.2.

I didn't find any calls from sessionstore.  I don't think it's ever used nsISHTransaction.
Comment 10 Olli Pettay [:smaug] 2010-11-24 09:20:38 PST
Ok, thanks. I think in that case it isn't absolutely critical to
take this to branches. But for trunk we really need this.
Comment 11 Boris Zbarsky (:bz) 2010-11-24 14:16:02 PST
Comment on attachment 492984 [details] [review]
patch, clear the pointers when modifying mListRoot

r=me
Comment 12 Olli Pettay [:smaug] 2010-11-26 02:26:24 PST
The patch applies cleanly to branches.
I'm not sure whether we really need it there, but should be safe.
Comment 13 Olli Pettay [:smaug] 2010-11-26 03:18:26 PST
http://hg.mozilla.org/mozilla-central/rev/92facbe5957e
Comment 14 Daniel Veditz 2010-11-29 10:42:45 PST
Comment on attachment 492984 [details] [review]
patch, clear the pointers when modifying mListRoot

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers
Comment 16 Olli Pettay [:smaug] 2011-01-04 04:11:03 PST
Backed out from branches, since there we some strange errors.
Not sure whether those were caused by the patch or by something else.
Comment 17 Olli Pettay [:smaug] 2011-01-04 04:13:15 PST
Seems like a valid problem. Investigating.
Comment 18 Olli Pettay [:smaug] 2011-01-04 04:18:29 PST
Ok, need to take one null check from Bug 534178 to branches.
Comment 19 Olli Pettay [:smaug] 2011-01-04 04:20:17 PST
Created attachment 500990 [details] [review]
null check aNext
Comment 20 Boris Zbarsky (:bz) 2011-01-04 20:23:57 PST
Comment on attachment 500990 [details] [review]
null check aNext

r=me
Comment 21 Christian Legnitto [:LegNeato] 2011-01-05 10:06:38 PST
Comment on attachment 500990 [details] [review]
null check aNext

Approving updated patch
Comment 22 Daniel Veditz 2011-01-07 10:45:40 PST
(In reply to comment #16)
> Backed out from branches, since there we some strange errors.
> Not sure whether those were caused by the patch or by something else.

This caused a huge crash spike, not something else
http://crash-stats.mozilla.com/products/Firefox/versions/3.6.14pre

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=nsSHTransaction%3A%3ASetNext%28nsISHTransaction*%29&version=Firefox%3A3.6.14pre

We definitely need the second patch with the null check.
Comment 23 Olli Pettay [:smaug] 2011-01-07 14:43:40 PST
Yeah. I'll push the null check patch this weekend.
Comment 25 Al Billings [:abillings] 2011-01-20 17:19:04 PST
Are there any STR for branches? I see that it is noted that the testcase doesn't work there.
Comment 26 Jesse Ruderman 2011-06-20 18:08:56 PDT
Crashtest: http://hg.mozilla.org/mozilla-central/rev/bed7a9570b40

Note You need to log in before you can comment on or make changes to this bug.