1290350 - Crash in OOM | large | NS_ABORT_OOM | nsSimpleURI::GetSpec

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-79169dbd-93e6-416e-a946-38c422160729.
=============================================================

This is currently the #243 topcrash in 47.0.1.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make fallible some potentially large allocations in nsSimpleURI::GetSpec

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8775868 [details] [diff] [review]
Make fallible some potentially large allocations in nsSimpleURI::GetSpec

Review of attachment 8775868 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, but I should probably defer to a netwerk reviewer. Looks like valentin is the main reviewer for this file (and mcmanus recognizes him as owner of URI code in bug 1247733 comment 28), so I'll tag this as feedback+ and punt the review to him.

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ PTO until Nov 5 }}]

Comment on attachment 8775868 [details] [diff] [review]
Make fallible some potentially large allocations in nsSimpleURI::GetSpec

Review of attachment 8775868 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/232d6b6ae7908041cdb09fff963990a1503d63d8
Bug 1290350 - Make fallible some potentially large allocations in nsSimpleURI::GetSpec. r=valentin.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/232d6b6ae790

Comment 6

[Nicholas Nethercote [inactive]]

I realize now that this patch is not the clear win I thought it was. Sure, nsSimplerURI::GetSpec() will return NS_ERROR_OUT_OF_MEMORY now instead of crashing. However, many calls to GetSpec() do not check their return value. If an OOM happens, they will proceed as if GetSpec() succeeded, and the effects will be unpredictable and depend on the call site. It's unlikely to cause crashes or security problems because it assigns to an nsCString, which just means the nsCString will be malformed (probably empty), but we could get erroneous behaviour.

In other words, one clearly-identifiable problem has likely been converted into an unknown number of unpredictable problems. It's certainly an argument for bug 1292440.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Njn, should we consider uplifting this to Aurora50? Based on comment 6, I wasn't sure whether we should.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Ritu Kothari (:ritu) from comment #7)
> Hi Njn, should we consider uplifting this to Aurora50? Based on comment 6, I
> wasn't sure whether we should.

I suggest holding off for now. I filed bug 1297300 to fix the problem. Once that lands, perhaps we can uplift them together.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tracking to make sure this gets uplifted at some point.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> (In reply to Ritu Kothari (:ritu) from comment #7)
> > Hi Njn, should we consider uplifting this to Aurora50? Based on comment 6, I
> > wasn't sure whether we should.
> 
> I suggest holding off for now. I filed bug 1297300 to fix the problem. Once
> that lands, perhaps we can uplift them together.

Makes sense. I marked bug 1297300 as status-firefox50 affected so both of these show up on the radar for Aurora50.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'OOM | large | NS_ABORT_OOM | nsSimpleURI::GetSpec':
 - nightly (version 51): 0 crashes from 2016-08-01.
 - aurora  (version 50): 0 crashes from 2016-08-01.
 - beta    (version 49): 0 crashes from 2016-08-02.
 - release (version 48): 796 crashes from 2016-07-25.
 - esr     (version 45): 796 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly       0       0       0
 - aurora        0       0       0
 - beta          0       0       0
 - release     265     186     118
 - esr          66      68      75

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly
 - aurora
 - beta
 - release #63       #232
 - esr     #103

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Looks like aurora is already fixed?!

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Looks like aurora is already fixed?!

It could be a different signature on Aurora: https://crash-stats.mozilla.com/search/?product=Firefox&proto_signature=~nsSimpleURI%3A%3AGetSpec&version=50.0a2&_sort=-date&_facets=signature&_facets=version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature.

Comment 14

[Nicholas Nethercote [inactive]]

> I suggest holding off for now. I filed bug 1297300 to fix the problem. Once
> that lands, perhaps we can uplift them together.

I'm making progress on bug 1297300, but it's slow. I don't think I will have fixed all the GetSpec() call sites by September 12, which is when this fix will naturally migrate to Aurora.

So we definitely shouldn't uplift this fix early. I'm on the fence about whether we should back this change out and only re-land it once bug 1297300 is complete.

Comment 15

[[:philipp]]

would we be able to uplift the fixes to 50 beta now that bug 1297300 is fixed as well?

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to [:philipp] from comment #15)
> would we be able to uplift the fixes to 50 beta now that bug 1297300 is
> fixed as well?

No. Bug 1297300 only ended up fixing half of the problem. Bug 1301607 is the follow-up to fix the remainder, but I haven't made any progress yet. At this point I think we should just let all these changes ride the trains.

Comment 17

[Julien Cristau [:jcristau]]

Marking wontfix for 50 per comment 16.