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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Doug Turner (:dougt)]

how often will people see this crash?

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1 bandaide

Comment 3

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/11638b731455

Comment 5

[Scoobidiver (away)]

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

Comment 6

[Josh Matthews [:jdm]]

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;

Comment 7

[Josh Matthews [:jdm]]

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

[Doug Turner (:dougt)]

I am not seeing this in fennec logs.  sdwilsh, thoughts?

Comment 9

[Olli Pettay [:smaug]]

Attachment: patch

Comment 10

[Olli Pettay [:smaug]]

Comment on attachment 516356 [details] [diff] [review]
patch

Better bandaid

Comment 11

[Josh Matthews [:jdm]]

The same fix is required in nsSHistory::EvictContentViewersInRange.

Comment 12

[Olli Pettay [:smaug]]

Attachment: +range

Comment 13

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 516369 [details] [diff] [review]
+range

OK, but why is that ending up null?

Comment 14

[Olli Pettay [:smaug]]

That is the non-bandaid part, which I'll fix.

Comment 15

[Olli Pettay [:smaug]]

...in a followup

Comment 16

[Olli Pettay [:smaug]]

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

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 516369 [details] [diff] [review]
+range

We can take this after Firefox 4.

Comment 18

[:ehsan akhgari]

Which patch here should land on cedar?

Comment 19

[Josh Matthews [:jdm]]

The patch entitled +range is the one which is ready and unlanded.

Comment 20

[Olli Pettay [:smaug]]

http://hg.mozilla.org/mozilla-central/rev/319a5c47e52d

Comment 21

[Robert Kaiser]

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).