Closed
Bug 1290350
Opened 8 years ago
Closed 8 years ago
Crash in OOM | large | NS_ABORT_OOM | nsSimpleURI::GetSpec
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
(Keywords: crash, Whiteboard: [necko-active])
Crash Data
Attachments
(1 file)
(deleted),
patch
|
valentin
:
review+
dholbert
:
feedback+
|
Details | Diff | Splinter Review |
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 | |
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8775868 -
Flags: review?(valentin.gosu)
Attachment #8775868 -
Flags: review?(dholbert)
Attachment #8775868 -
Flags: feedback+
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!
Attachment #8775868 -
Flags: review?(valentin.gosu) → review+
![]() |
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.
Updated•8 years ago
|
Whiteboard: [necko-active]
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/232d6b6ae790
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
![]() |
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.
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•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.
Flags: needinfo?(n.nethercote)
Comment 9•8 years ago
|
||
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.
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
status-firefox48:
--- → affected
status-firefox-esr45:
--- → affected
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.
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•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.
Comment 15•8 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•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.
Flags: needinfo?(n.nethercote)
You need to log in
before you can comment on or make changes to this bug.
Description
•