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

RESOLVED FIXED in Firefox 51

Status

()

Core
Networking
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

({crash})

Trunk
mozilla51
x86
Windows 7
crash
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox-esr45 wontfix, firefox50+ wontfix, firefox51 fixed)

Details

(Whiteboard: [necko-active], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

2 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.
(Assignee)

Comment 1

2 years ago
Created attachment 8775868 [details] [diff] [review]
Make fallible some potentially large allocations in nsSimpleURI::GetSpec
Attachment #8775868 - Flags: review?(dholbert)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8775868 - Flags: review?(valentin.gosu)
Attachment #8775868 - Flags: review?(dholbert)
Attachment #8775868 - Flags: feedback+
Comment on attachment 8775868 [details] [diff] [review]
Make fallible some potentially large allocations in nsSimpleURI::GetSpec

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

Thanks!
Attachment #8775868 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/232d6b6ae7908041cdb09fff963990a1503d63d8
Bug 1290350 - Make fallible some potentially large allocations in nsSimpleURI::GetSpec. r=valentin.
Whiteboard: [necko-active]

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/232d6b6ae790
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 6

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

Comment 7

2 years ago
Hi Njn, should we consider uplifting this to Aurora50? Based on comment 6, I wasn't sure whether we should.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 8

2 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.
Flags: needinfo?(n.nethercote)
Tracking to make sure this gets uplifted at some point.
tracking-firefox50: --- → +
(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.
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
status-firefox48: --- → affected
status-firefox-esr45: --- → affected
Looks like aurora is already fixed?!
status-firefox48: affected → wontfix
status-firefox-esr45: affected → wontfix
(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.
Crash Signature: [@ OOM | large | NS_ABORT_OOM | nsSimpleURI::GetSpec] → [@ OOM | large | NS_ABORT_OOM | nsSimpleURI::GetSpec] [@ OOM | large | NS_ABORT_OOM | mozilla::net::nsSimpleURI::GetSpec]
(Assignee)

Comment 14

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

Comment 15

2 years ago
would we be able to uplift the fixes to 50 beta now that bug 1297300 is fixed as well?
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 16

2 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.
Flags: needinfo?(n.nethercote)
Marking wontfix for 50 per comment 16.
status-firefox50: affected → wontfix
You need to log in before you can comment on or make changes to this bug.