Closed Bug 614499 Opened 9 years ago Closed 9 years ago

Crash [@ nsSHTransaction::GetPrev ]

Categories

(Core :: DOM: Navigation, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical] testcase doesn't affect < FF4 [qa-needs-STR])

Crash Data

Attachments

(3 files, 1 obsolete file)

This might be the same bug as topcrash bug 612881 / bug 612887.
Attached file stack trace
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.
blocking2.0: --- → ?
Is this win7 only?  It doesn't crash my Linux x64 box.

Will definitely look at this tomorrow.
Debug builds crash on Windows, Mac, and Linux.  Opt builds crash on at least Windows and Mac.
Assignee: nobody → Olli.Pettay
Justin, I took this before noticing your comment.
I assume this might be a regression from my changes.
Ah, the code causing this has been there for ages, at least from year 2001.
Attachment #492984 - Flags: review?(bzbarsky)
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.
(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.
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 on attachment 492984 [details] [diff] [review]
patch, clear the pointers when modifying mListRoot

r=me
Attachment #492984 - Flags: review?(bzbarsky) → review+
Attachment #492984 - Flags: approval2.0?
Attachment #492984 - Flags: approval2.0?
Summary: Crash [@ nsSHTransaction::GetPrev] → Crash [@ nsSHTransaction::GetPrev ]
The patch applies cleanly to branches.
I'm not sure whether we really need it there, but should be safe.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
http://hg.mozilla.org/mozilla-central/rev/92facbe5957e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
Comment on attachment 492984 [details] [diff] [review]
patch, clear the pointers when modifying mListRoot

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers
Attachment #492984 - Flags: approval1.9.2.14+
Attachment #492984 - Flags: approval1.9.1.17+
Whiteboard: [sg:critical] → [sg:critical] testcase doesn't affect < FF4
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
blocking1.9.1: needed → .17+
blocking1.9.2: needed → .14+
Backed out from branches, since there we some strange errors.
Not sure whether those were caused by the patch or by something else.
Seems like a valid problem. Investigating.
Ok, need to take one null check from Bug 534178 to branches.
Attached patch null check aNextSplinter Review
Attachment #500990 - Flags: review?(bzbarsky)
Attachment #500990 - Flags: approval1.9.2.14?
Attachment #500990 - Flags: approval1.9.1.17?
No longer blocks: 612881
Blocks: 612881
Comment on attachment 500990 [details] [diff] [review]
null check aNext

r=me
Attachment #500990 - Flags: review?(bzbarsky) → review+
Comment on attachment 500990 [details] [diff] [review]
null check aNext

Approving updated patch
Attachment #500990 - Flags: approval1.9.2.14?
Attachment #500990 - Flags: approval1.9.2.14+
Attachment #500990 - Flags: approval1.9.1.17?
Attachment #500990 - Flags: approval1.9.1.17+
Attachment #492984 - Flags: approval1.9.2.14-
Attachment #492984 - Flags: approval1.9.2.14+
Attachment #492984 - Flags: approval1.9.1.17-
Attachment #492984 - Flags: approval1.9.1.17+
Attachment #492984 - Attachment is obsolete: true
(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.
Yeah. I'll push the null check patch this weekend.
Are there any STR for branches? I see that it is noted that the testcase doesn't work there.
Whiteboard: [sg:critical] testcase doesn't affect < FF4 → [sg:critical] testcase doesn't affect < FF4 [qa-needs-STR]
Group: core-security
Crash Signature: [@ nsSHTransaction::GetPrev ]
Crashtest: http://hg.mozilla.org/mozilla-central/rev/bed7a9570b40
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.