Closed
Bug 624758
Opened 15 years ago
Closed 14 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•15 years ago
|
tracking-fennec: --- → ?
Comment 1•15 years ago
|
||
how often will people see this crash?
Updated•15 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•15 years ago
|
Assignee: nobody → doug.turner
Comment 2•14 years ago
|
||
Attachment #507262 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 years ago
|
Attachment #507262 -
Flags: review?(Olli.Pettay) → review+
Comment 3•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 5•14 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•14 years ago
|
OS: Linux → All
Version: unspecified → Trunk
Reporter | ||
Comment 6•14 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•14 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•14 years ago
|
||
I am not seeing this in fennec logs. sdwilsh, thoughts?
tracking-fennec: 2.0+ → ---
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #516356 -
Flags: review?(doug.turner)
Updated•14 years ago
|
Assignee: doug.turner → nobody
Updated•14 years ago
|
Attachment #516356 -
Flags: review?(doug.turner) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #516356 -
Flags: approval2.0?
Assignee | ||
Comment 10•14 years ago
|
||
Comment on attachment 516356 [details] [diff] [review]
patch
Better bandaid
Attachment #516356 -
Flags: superreview?(bzbarsky)
Reporter | ||
Comment 11•14 years ago
|
||
The same fix is required in nsSHistory::EvictContentViewersInRange.
Assignee | ||
Comment 12•14 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•14 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•14 years ago
|
||
That is the non-bandaid part, which I'll fix.
Assignee | ||
Comment 15•14 years ago
|
||
...in a followup
Assignee | ||
Comment 16•14 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•14 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•14 years ago
|
Whiteboard: not-ready
Reporter | ||
Comment 19•14 years ago
|
||
The patch entitled +range is the one which is ready and unlanded.
Assignee | ||
Comment 20•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
![]() |
||
Comment 21•14 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•14 years ago
|
Crash Signature: [@nsSHistory::EvictContentViewersInRange]
[@nsSHistory::EvictGlobalContentViewer]
Updated•14 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
•