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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(3 files, 1 obsolete file)

ensureLinear allows a null cx to be passed in, in which case it eats OOM reports.
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)
Pass through cx in many more places that may end up flattening a string and thus want OOM reporting.
Attachment #8952556 - Flags: review?(jcoppeard)
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 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).
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 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)
Priority: -- → P3
(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.
Attached patch CTypes string error checking (obsolete) — Splinter Review
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.
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)
Attachment #8953658 - Attachment is obsolete: true
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+
Attachment #8952556 - Flags: review?(jcoppeard) → review+
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
https://hg.mozilla.org/mozilla-central/rev/321ec4ba2ea3
https://hg.mozilla.org/mozilla-central/rev/d56f8af6e3ab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.