Closed Bug 624758 Opened 9 years ago Closed 9 years ago

crash [@nsSHistory::EvictContentViewersInRange] and [@nsSHistory::EvictGlobalContentViewer] don't handle failure of GetTransactionAtIndex

Categories

(Core :: DOM: Navigation, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jdm, Assigned: smaug)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
how often will people see this crash?
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
Assignee: nobody → doug.turner
Attachment #507262 - Flags: review?(Olli.Pettay)
Attachment #507262 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/11638b731455
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 608218
OS: Linux → All
Version: unspecified → Trunk
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;
Actually, scratch what I just wrote, I was thinking of GetTransactionAtIndex.  GetNext always returns NS_OK, so a null-check is what we need.
I am not seeing this in fennec logs.  sdwilsh, thoughts?
tracking-fennec: 2.0+ → ---
Attached patch patch (obsolete) — Splinter Review
Attachment #516356 - Flags: review?(doug.turner)
Assignee: doug.turner → nobody
Attachment #516356 - Flags: review?(doug.turner) → review+
Attachment #516356 - Flags: approval2.0?
Comment on attachment 516356 [details] [diff] [review]
patch

Better bandaid
Attachment #516356 - Flags: superreview?(bzbarsky)
The same fix is required in nsSHistory::EvictContentViewersInRange.
Attached patch +rangeSplinter Review
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 on attachment 516369 [details] [diff] [review]
+range

OK, but why is that ending up null?
Attachment #516369 - Flags: review?(bzbarsky) → review+
That is the non-bandaid part, which I'll fix.
...in a followup
Btw, this is not a regression from 1.9.2.
There are similar crashes on 3.6.x, and even 3.5.x
Comment on attachment 516369 [details] [diff] [review]
+range

We can take this after Firefox 4.
Attachment #516369 - Flags: approval2.0? → approval2.0-
Whiteboard: not-ready
Which patch here should land on cedar?
Whiteboard: not-ready
The patch entitled +range is the one which is ready and unlanded.
http://hg.mozilla.org/mozilla-central/rev/319a5c47e52d
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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).
Crash Signature: [@nsSHistory::EvictContentViewersInRange] [@nsSHistory::EvictGlobalContentViewer]
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.