Closed
Bug 737164
Opened 12 years ago
Closed 12 years ago
Make strings malloc-infallible by default
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
(Keywords: sec-want)
Attachments
(6 files, 1 obsolete file)
25.29 KB,
patch
|
justin.lebar+bug
:
review+
dbaron
:
feedback+
|
Details | Diff | Splinter Review |
2.76 KB,
text/plain
|
jduell.mcbugs
:
review+
|
Details |
3.85 KB,
text/plain
|
Details | |
2.54 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
2.57 KB,
patch
|
Details | Diff | Splinter Review | |
1.46 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Our current string API is supposedly malloc-safe, but in practice this means that callers don't check return values and so many callers are not actually OOM-safe. In this bug I intend to make the default version of all API calls infallible (abort on OOM) and provide an explicitly fallible version the same way we do for "operator new": bool NS_WARN_UNUSED_RESULT Assign(const nsSubstring_type& other, const mozilla::fallible_t&);
Assignee | ||
Comment 1•12 years ago
|
||
I will do the subclasses (nsTString etc) as a separate patch for ease of commit and review. This could be committed as-is with the minor tree fixups in part B/C
Attachment #607282 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #607282 -
Flags: feedback?(dbaron)
Comment 4•12 years ago
|
||
> bool NS_WARN_UNUSED_RESULT Assign(const nsSubstring_type& other, const mozilla::fallible_t&); What's the advantage of this as opposed to > bool NS_WARN_UNUSED_RESULT FallibleAssign(const nsSubstring_type& other) ? I guess it's nice that fallible_t() matches our fallible operator new, but perhaps new should be the exception, rather than the rule?
Comment 5•12 years ago
|
||
Why aren't there infallible versions of EnsureMutable and ReplacePrep? Seems like that would save a bunch of if statements.
Assignee | ||
Comment 6•12 years ago
|
||
I don't think that having alternately-named methods would be more readable: it's nice (to me at least!) to have fallible_t as the trigger for reviewers and coders that a method needs extra attention. EnsureMutable and ReplacePrep were both internal (protected) methods, so I was mainly doing the expedient thing with them. If you think it would generate faster code to make an infallible version and remove some of the checks, I can try it: I'd need to look at the generated code and make sure that passing fallible_t doesn't actually run extra code in optimized builds; I'd expect that the caller would have to allocate stack space for the parameter but that neither side actually needs to touch that stack space. I'd be very impressed and happy if MSVC LTCG could remove the parameter altogether.
Comment 7•12 years ago
|
||
> I don't think that having alternately-named methods would be more readable: it's nice (to me at > least!) to have fallible_t as the trigger for reviewers and coders that a method needs extra > attention. It's not a big deal either way. > If you think it would generate faster code to make an infallible version and remove some of the > checks, I can try it I suppose it's possible that code would run faster if you pushed down the if statement into EnsureMutable / ReplacePrep -- it's less work for the branch predictor, assuming they don't get inlined. But I doubt this matters; I was mainly speaking from a cleanliness POV -- the fewer places in the code where you handle OOM, the better. It probably wouldn't be a bad idea to annotate the OOM if statement as unlikely, though. > I'd be very impressed and happy if MSVC LTCG could remove the parameter altogether. Hopefully most of the calls you care about being fast are inlined anyway.
Comment 8•12 years ago
|
||
> + // There are not fallible version of these methods because they only really
> + // apply to small allocations that we wouldn't want to check anyway.
Nit: s/really//
My preference would still be to push the aborts down into EnsureMutable / ReplacePrep, unless there's a good reason not to, so things like the unlikely branch annotation are easier to write. But r=me either way.
Updated•12 years ago
|
Attachment #607282 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #619991 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 10•12 years ago
|
||
Bah, need C++ include guard.
Attachment #619998 -
Flags: review?(jones.chris.g)
Comment on attachment 619998 [details] [diff] [review] Move the declaration of fallible_t to its own header, rev. 1.1 I'm not sure which patch of these patches I was supposed to review, but ...
Attachment #619998 -
Flags: review?(jones.chris.g)
Comment on attachment 619991 [details] [diff] [review] Move the declaration of fallible_t to its own header, rev. 1 ... I prefer leaving the #include at the top of the file. Please put it in the #if defined(__cplusplug) just above here. r=me with that
Attachment #619991 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #607284 -
Flags: review?(jduell.mcbugs)
Comment 13•12 years ago
|
||
Comment on attachment 607284 [details] Part B - necko changes, checkpoint I think we want to keep NS_ReadInputStreamToString fallible, at least for now. Websockets uses it to slurp up arbitrarily large blobs: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#359 Other places where we use the function to read in whole (but prob small enough) files: mozJSSubScriptLoader::ReadScript(), nsFrameScriptExecutor::LoadFrameScriptInternal().
Comment 14•12 years ago
|
||
Websockets will no longer slurp whole blob in once bug 704447 is fixed, BTW.
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39e55d53e0a3 https://hg.mozilla.org/mozilla-central/rev/b6d494a57122 https://hg.mozilla.org/mozilla-central/rev/971a0d129ff3
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Comment 17•12 years ago
|
||
We landed the necko patch and closed this w/o addressing comment 13. Everything that's checked in is fine, but I do think we also need to make the ReadInputStreamToString change.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•12 years ago
|
||
Comment on attachment 623066 [details] [diff] [review] Make ReadInputStreamToString fallible sorry, I somehow got confused on which patches were reviewed :-(
Attachment #623066 -
Flags: review?(benjamin) → review+
Comment 21•12 years ago
|
||
So my patch burned up the inbound tree. The issue is that nsNetUtil.h is included and used by toolkit/system/gnome/nsAlertsIconListener.cpp which doesn't define MOZILLA_INTERNAL_API. This patch fixes by wrapping just the NS_ReadInputStreamToString function in #ifdef MOZILLA_INTERNAL_API. Alternately we could have sAlertsIconListener.cpp stop using NS_NewURI. We're not changing the external string API to be infallible, are we?
Comment 22•12 years ago
|
||
Attachment #623066 -
Attachment is obsolete: true
Attachment #623357 -
Flags: review?(benjamin)
Assignee | ||
Comment 23•12 years ago
|
||
Comment on attachment 623357 [details] [diff] [review] Make ReadInputStreamToString fallible, v2 The external API has become infallible because it basically forwards to the internal API.
Attachment #623357 -
Flags: review?(benjamin) → review+
Comment 25•12 years ago
|
||
Comment on attachment 607284 [details]
Part B - necko changes, checkpoint
belated +r to already-landed patch
Attachment #607284 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 26•12 years ago
|
||
Requesting tracking because this has the possibility of causing new crashes (with mozalloc_abort | * signatures) if there are places which currently may have content-controlled large allocations and do have string API checks already in place.
tracking-firefox15:
--- → ?
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bdd1e7c33358
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
tracking-firefox15:
? → ---
Comment 28•12 years ago
|
||
marking for tracking as per comment 26, if there are regressions please update the status flag so this gets on our radar again.
status-firefox15:
--- → fixed
tracking-firefox15:
--- → +
Attachment #607282 -
Flags: feedback?(dbaron) → feedback+
Blocks: 1027163
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•