Closed
Bug 1439712
Opened 6 years ago
Closed 6 years ago
ensureLinear does not report OOM when called from JIT
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
Details
Attachments
(3 files, 1 obsolete file)
5.15 KB,
patch
|
Details | Diff | Splinter Review | |
35.24 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
31.36 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
ensureLinear allows a null cx to be passed in, in which case it eats OOM reports.
Assignee | ||
Comment 1•6 years ago
|
||
This may be absolutely the wrong way to go about it. I could do a TLS lookup from within ensureLinear if needed, though I'm not sure of the thread assumptions in that code.
Attachment #8952496 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•6 years ago
|
||
Pass through cx in many more places that may end up flattening a string and thus want OOM reporting.
Attachment #8952556 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 8952496 [details] [diff] [review] Pass cx from JIT into ensureLinear() during string comparison to report OOM when linearizing a rope Review of attachment 8952496 [details] [diff] [review]: ----------------------------------------------------------------- Actually, after having written the followup patch, it doesn't look doable to make all callers pass in a cx. So the alternative to this patch would be a TlsContext() in EqualStringsHelper.
Comment 4•6 years ago
|
||
Comment on attachment 8952556 [details] [diff] [review] Improve JSString::ensureLinear error reporting Review of attachment 8952556 [details] [diff] [review]: ----------------------------------------------------------------- Everything looks fine except the ctypes changes. I don't understand how the error reporting works there. It looks like we just ignore errors flattening the string. In that case we could have a successful operation that reports out of memory. That seems broken. ::: js/src/jsapi.cpp @@ -6080,5 @@ > return PutEscapedString(buffer, size, linearStr, quote); > } > > -JS_PUBLIC_API(bool) > -JS_FileEscapedString(FILE* fp, JSString* str, char quote) This is a weird name. I think this should be called WriteEscapedString (but this is not related to this bug).
Comment 5•6 years ago
|
||
Note that the JIT case is by design (there should have been comments there): if the IC stub fails, we jump to the next stub and eventually the C++ code. We don't want to have a pending exception on the cx because we're not in "propagating an exception" mode.
Comment 6•6 years ago
|
||
Comment on attachment 8952496 [details] [diff] [review] Pass cx from JIT into ensureLinear() during string comparison to report OOM when linearizing a rope Clearing per comment 5.
Attachment #8952496 -
Flags: review?(jdemooij)
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5) > Note that the JIT case is by design (there should have been comments there): > if the IC stub fails, we jump to the next stub and eventually the C++ code. > We don't want to have a pending exception on the cx because we're not in > "propagating an exception" mode. Ah! That makes a lot of sense, but I wouldn't have guessed it. I'll add a comment, thanks.
Assignee | ||
Comment 8•6 years ago
|
||
You're right, it looks like string building is unchecked all over the place in ctypes. Ok, this is a patch that applies on top of the other patch. I will collapse them before landing, but it's probably easiest to see this as an interdiff. Oh, wait. I won't ask for review yet -- I need to comment a nontrivial change.
Assignee | ||
Comment 9•6 years ago
|
||
I'm not confident ctypes does reasonable things when it detects errors, but at least this now does detect the errors.
Attachment #8953662 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•6 years ago
|
Attachment #8953658 -
Attachment is obsolete: true
Comment 10•6 years ago
|
||
Comment on attachment 8953662 [details] [diff] [review] CTypes string error checking Review of attachment 8953662 [details] [diff] [review]: ----------------------------------------------------------------- Wow, sorry I made you write all this. But it's good to know we actually check now. ::: js/src/ctypes/CTypes.cpp @@ +928,5 @@ > return obj; > } > > static MOZ_ALWAYS_INLINE JSString* > +NewUCString(JSContext* cx, const AutoStringChars&& from) Might it be easier to pass a normal reference to the string builder all the way down? ::: js/src/ctypes/CTypes.h @@ +35,5 @@ > +class StringBuilder { > + Vector<CharT, N, SystemAllocPolicy> v; > + > + // Have any (OOM) errors been encountered while constructing this string? > + bool errored { false }; This confused me until I realised that it's member initialisation with braces. @@ +41,5 @@ > + // Have we finished building this string? > + bool finished { false }; > + > + // Did we check for errors? > + mutable bool checked { false }; Probably some of these could be debug-only.
Attachment #8953662 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8952556 -
Flags: review?(jcoppeard) → review+
Comment 11•6 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/321ec4ba2ea3 Improve JSString::ensureLinear error reporting, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/d56f8af6e3ab CTypes string error checking, r=jonco
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/321ec4ba2ea3 https://hg.mozilla.org/mozilla-central/rev/d56f8af6e3ab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•