Nicholas Nethercote [inactive] Assignee
	Description • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 2 • 8 years ago

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.

	Valentin Gosu [:valentin] (he/him)
	Comment 3 • 8 years ago

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

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

Thanks!

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 5 • 8 years ago
bugherder

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

	Nicholas Nethercote [inactive] Assignee
	Comment 6 • 8 years ago

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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 7 • 8 years ago

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

	Nicholas Nethercote [inactive] Assignee
	Comment 8 • 8 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 9 • 8 years ago

Tracking to make sure this gets uplifted at some point.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 8 years ago

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

	BugBot [:suhaib / :marco/ :calixte]
	Comment 11 • 8 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 12 • 8 years ago

Looks like aurora is already fixed?!

	Marco Castelluccio [:marco]
	Comment 13 • 8 years ago

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

	Nicholas Nethercote [inactive] Assignee
	Comment 14 • 8 years ago

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

	[:philipp]
	Comment 15 • 8 years ago

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

	Nicholas Nethercote [inactive] Assignee
	Comment 16 • 8 years ago

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

	Julien Cristau [:jcristau]
	Comment 17 • 8 years ago

Marking wontfix for 50 per comment 16.