Closed
Bug 624758
Opened 14 years ago
Closed 13 years ago
crash [@nsSHistory::EvictContentViewersInRange] and [@nsSHistory::EvictGlobalContentViewer] don't handle failure of GetTransactionAtIndex
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jdm, Assigned: smaug)
References
Details
(Keywords: crash)
Crash Data
Attachments
(2 files, 1 obsolete file)
1.95 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.06 KB,
patch
|
bzbarsky
:
review+
beltzner
:
approval2.0-
|
Details | Diff | Splinter Review |
I came across this after seeing a couple Fennec crashes at nsSHistory.cpp:894 with a null pointer: >886 void >887 nsSHistory::EvictContentViewersInRange(PRInt32 aStart, PRInt32 aEnd) >888 { >889 nsCOMPtr<nsISHTransaction> trans; >890 GetTransactionAtIndex(aStart, getter_AddRefs(trans)); ... >894 trans->GetSHEntry(getter_AddRefs(entry)); GetTransactionAtIndex (http://hg.mozilla.org/mozilla-central/annotate/88db8ccdd0de/docshell/shistory/src/nsSHistory.cpp#l419) looks like it sets a non-null outref if it returns NS_OK, as it unconditionally NS_ADDREFs the result, so we'd see a different crash if it were otherwise. Therefore, we appear to be neglecting to handle failures in crashes such as http://crash-stats.mozilla.com/report/index/71d8c849-32b8-4f20-aa79-dd13b2110110 and http://crash-stats.mozilla.com/report/index/9658d2f0-102a-4150-bf2d-ec1b12101225.
Reporter | ||
Updated•14 years ago
|
tracking-fennec: --- → ?
Comment 1•14 years ago
|
||
how often will people see this crash?
Updated•13 years ago
|
tracking-fennec: ? → 2.0+
Summary: nsSHistory::EvictContentViewersInRange and EvictGlobalContentViewer don't handle failure of GetTransactionAtIndex → crash [@nsSHistory::EvictContentViewersInRange] and [@nsSHistory::EvictGlobalContentViewer] don't handle failure of GetTransactionAtIndex
Updated•13 years ago
|
Assignee: nobody → doug.turner
Comment 2•13 years ago
|
||
Attachment #507262 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
Attachment #507262 -
Flags: review?(Olli.Pettay) → review+
Comment 3•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/11638b731455
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 5•13 years ago
|
||
It still crashes. See: * Windows: https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=4&range_unit=weeks&signature=nsSHistory%3A%3AEvictGlobalContentViewer%28%29 * Mac OS X/Linux: https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=4&range_unit=weeks&signature=nsSHistory%3A%3AEvictGlobalContentViewer https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=4&range_unit=weeks&signature=nsSHistory::EvictContentViewersInRange
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•13 years ago
|
OS: Linux → All
Version: unspecified → Trunk
Reporter | ||
Comment 6•13 years ago
|
||
Doug, we've got:
> 974 if (!trans) {
> 975 shist = static_cast<nsSHistory*>(PR_NEXT_LINK(shist));
> 976 continue;
> 977 }
> 978
> 979 for (PRInt32 i = startIndex; i <= endIndex; ++i) {
> 980 nsCOMPtr<nsISHEntry> entry;
> 981 trans->GetSHEntry(getter_AddRefs(entry));
> ...
>1035 nsISHTransaction* temp = trans;
>1036 temp->GetNext(getter_AddRefs(trans));
>1037 }
>1038 shist = static_cast<nsSHistory*>(PR_NEXT_LINK(shist));
in both files, so the null check needs to happen within the for loop. Actually, looks like it could be:
nsresult rv = temp->GetNext(getter_AddRefs(trans));
if (NS_FAILED(rv))
break;
Reporter | ||
Comment 7•13 years ago
|
||
Actually, scratch what I just wrote, I was thinking of GetTransactionAtIndex. GetNext always returns NS_OK, so a null-check is what we need.
Comment 8•13 years ago
|
||
I am not seeing this in fennec logs. sdwilsh, thoughts?
tracking-fennec: 2.0+ → ---
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #516356 -
Flags: review?(doug.turner)
Updated•13 years ago
|
Assignee: doug.turner → nobody
Updated•13 years ago
|
Attachment #516356 -
Flags: review?(doug.turner) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #516356 -
Flags: approval2.0?
Assignee | ||
Comment 10•13 years ago
|
||
Comment on attachment 516356 [details] [diff] [review] patch Better bandaid
Attachment #516356 -
Flags: superreview?(bzbarsky)
Reporter | ||
Comment 11•13 years ago
|
||
The same fix is required in nsSHistory::EvictContentViewersInRange.
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #516356 -
Attachment is obsolete: true
Attachment #516369 -
Flags: review?(bzbarsky)
Attachment #516369 -
Flags: approval2.0?
Attachment #516356 -
Flags: superreview?(bzbarsky)
Attachment #516356 -
Flags: approval2.0?
Comment 13•13 years ago
|
||
Comment on attachment 516369 [details] [diff] [review] +range OK, but why is that ending up null?
Attachment #516369 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 14•13 years ago
|
||
That is the non-bandaid part, which I'll fix.
Assignee | ||
Comment 15•13 years ago
|
||
...in a followup
Assignee | ||
Comment 16•13 years ago
|
||
Btw, this is not a regression from 1.9.2. There are similar crashes on 3.6.x, and even 3.5.x
Comment 17•13 years ago
|
||
Comment on attachment 516369 [details] [diff] [review] +range We can take this after Firefox 4.
Attachment #516369 -
Flags: approval2.0? → approval2.0-
Updated•13 years ago
|
Whiteboard: not-ready
Reporter | ||
Comment 19•13 years ago
|
||
The patch entitled +range is the one which is ready and unlanded.
Assignee | ||
Comment 20•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/319a5c47e52d
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 21•13 years ago
|
||
Is bug 612274 indeed a dupe of this? If so, that one is #243 and over 20 crashes per ADU now on 4.0* - if the fix is low-risk, I wonder if we might want to still get it into Macaw (would need to go via nomination etc. to landing very soon to make that).
Updated•13 years ago
|
Crash Signature: [@nsSHistory::EvictContentViewersInRange]
[@nsSHistory::EvictGlobalContentViewer]
Updated•13 years ago
|
Assignee: nobody → Olli.Pettay
Crash Signature: [@nsSHistory::EvictContentViewersInRange]
[@nsSHistory::EvictGlobalContentViewer] → [@nsSHistory::EvictContentViewersInRange]
[@nsSHistory::EvictGlobalContentViewer]
You need to log in
before you can comment on or make changes to this bug.
Description
•