1330536 - Add a way to annotate Web IDL things as throwing OOM only

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Throwing things can't be code-moved, in general, because exceptions happening out of order with other stuff is observable.

But that's only really true for exceptions that are specced to happen or are otherwise reliable.  OOM doesn't count.  So we can relax the restriction and allow code motion of things that can only throw OOM.

Patches coming up; the complexity (for which I apologize) is due to me wanting to enforce at compile time that people can't screw this up by saying they only throw OOM and throwing something else.

With these patches, the testcase linked from bug 1330475 loop-hoists the textContent bits.

Comment 1

[Boris Zbarsky [:bzbarsky]]

jdm, ms2ger, servo may want something like this too.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a CanOOM annotation, and the corresponding GetterCanOOM, SetterCanOOM annotations, to the IDL parser. These can be used in the cases when Throws/GetterThrows/SetterThrows can be used, to indicate that the only possible exception is NS_ERROR_

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change Descriptor.getExtendedAttributes to include 'canOOM' information as needed

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a subclass of FastErrorResult that can be used only to throw OOM

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Pass OOMReporter from bindings in cases that can OOM but are otherwise infallible

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Correctly mark fallibility/movability/etc in jitinfo for cases that can OOM but are otherwise infallible

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Add tests for code generation for CanOOM members

Some examples of the resulting codegen.  First of all, the example generation:

  void CanOOMMethod(OOMReporter& aRv);
  bool GetCanOOMAttr(OOMReporter& aRv) const;
  void SetCanOOMAttr(bool arg, OOMReporter& aRv);
  bool GetCanOOMGetterAttr(OOMReporter& aRv) const;
  void SetCanOOMGetterAttr(bool arg);
  bool CanOOMSetterAttr() const;
  void SetCanOOMSetterAttr(bool arg, OOMReporter& aRv);

Now the implementation of the caller of CanOOMMethod:

  static bool
  canOOMMethod(JSContext* cx, JS::Handle<JSObject*> obj, mozilla::dom::TestExampleInterface* self, const JSJitMethodCallArgs& args)
  {
    binding_danger::OOMReporterInstantiator rv;
    self->CanOOMMethod(rv);
    if (MOZ_UNLIKELY(rv.MaybeSetPendingException(cx))) {
      return false;
    }
    MOZ_ASSERT(!JS_IsExceptionPending(cx));
    args.rval().setUndefined();
    return true;
  }

  static const JSJitInfo canOOMMethod_methodinfo = {
    { (JSJitGetterOp)canOOMMethod },
    { prototypes::id::TestExampleInterface },
    { PrototypeTraits<prototypes::id::TestExampleInterface>::Depth },
    JSJitInfo::Method,
    JSJitInfo::AliasEverything, /* aliasSet.  Not relevant for setters. */
    JSVAL_TYPE_UNDEFINED,  /* returnType.  Not relevant for setters. */
    false,  /* isInfallible. False in setters. */
    false,  /* isMovable.  Not relevant for setters. */
    false, /* isEliminatable.  Not relevant for setters. */
    false, /* isAlwaysInSlot.  Only relevant for getters. */
    false, /* isLazilyCachedInSlot.  Only relevant for getters. */
    false,  /* isTypedMethod.  Only relevant for methods. */
    0   /* Reserved slot index, if we're stored in a slot, else 0. */
  };

and I have verified that isInfallible is false in canOOMAttr_getterInfo and
canOOMAttr_setterinfo (because from the JS engine point of view they _are_
fallible: they can throw OOM), but is true in canOOMSetterAttr_getterinfo.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Change nsINode::GetTextContent to take an OOMReporter, not an ErrorResult

This compiles, which shows that the hackery to allow passing an ErrorResult as an OOMReporter is working.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Annotate Node.textContent as capable of OOM but not of throwing otherwise in the IDL

Codegen for the textContent getter (no surprises there):

  static bool
  get_textContent(JSContext* cx, JS::Handle<JSObject*> obj, nsINode* self, JSJitGetterCallArgs args)
  {
    binding_danger::OOMReporterInstantiator rv;
    DOMString result;
    self->GetTextContent(result, rv);
    if (MOZ_UNLIKELY(rv.MaybeSetPendingException(cx))) {
      return false;
    }
    MOZ_ASSERT(!JS_IsExceptionPending(cx));
    if (!xpc::StringToJsval(cx, result, args.rval())) {
      return false;
    }
    return true;
  }

  static const JSJitInfo textContent_getterinfo = {
    { (JSJitGetterOp)get_textContent },
    { prototypes::id::Node },
    { PrototypeTraits<prototypes::id::Node>::Depth },
    JSJitInfo::Getter,
    JSJitInfo::AliasDOMSets, /* aliasSet.  Not relevant for setters. */
    JSVAL_TYPE_UNKNOWN,  /* returnType.  Not relevant for setters. */
    false,  /* isInfallible. False in setters. */
    true,  /* isMovable.  Not relevant for setters. */
    true, /* isEliminatable.  Not relevant for setters. */
    false, /* isAlwaysInSlot.  Only relevant for getters. */
    false, /* isLazilyCachedInSlot.  Only relevant for getters. */
    false,  /* isTypedMethod.  Only relevant for methods. */
    0   /* Reserved slot index, if we're stored in a slot, else 0. */
  };

Note that the jitinfo says this is fallible (both because canOOM, and because
its return value conversion can OOM already), but is movable and eliminatable,
which is exactly what we wanted.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that there are various other things that might benefit from this.  I did not look at things that are [Pure, Throws] very carefully, but I did look at some things that throw NS_ERROR_OUT_OF_MEMORY and I think DOMMatrixReadOnly::ToFloat32Array/ToFloat64Array could be [Pure, CanOOM] instead of [Throws].  I can look for other things that would benefit, if we decide we want to do this.

Comment 11

[Olli Pettay [:smaug]]

Comment on attachment 8826076 [details] [diff] [review]
part 2.  Change Descriptor.getExtendedAttributes to include 'canOOM' information as needed

>diff --git a/dom/bindings/Configuration.py b/dom/bindings/Configuration.py
>--- a/dom/bindings/Configuration.py
>+++ b/dom/bindings/Configuration.py
>@@ -563,45 +563,62 @@ class Descriptor(DescriptorProvider):
>     @property
>     def hasNamedPropertiesObject(self):
>         if self.interface.isExternal():
>             return False
> 
>         return self.isGlobal() and self.supportsNamedProperties()
> 
>     def getExtendedAttributes(self, member, getter=False, setter=False):
>+        def ensureValidBoolExtendedAttribute(attr, name):
>+            if (attr is not None and attr is not True):
>+                raise TypeError("Unknown value for '%s': %s" % (name, attr[0]))
>+
>         def ensureValidThrowsExtendedAttribute(attr):
>-            if (attr is not None and attr is not True):
>-                raise TypeError("Unknown value for 'Throws': " + attr[0])
>+            ensureValidBoolExtendedAttribute(attr, "Throws")
>+
>+        def ensureValidCanOOMExtendedAttribute(attr):
>+            ensureValidThrowsExtendedAttribute(attr, "CanOOM")
so you pass "CanOOM" to ensureValidThrowsExtendedAttribute, but ensureValidThrowsExtendedAttribute doesn't use it, but passes "Throws" to ensureValidBoolExtendedAttribute
Doesn't this mean ensureValidCanOOMExtendedAttribute throws, or am I missing something?
I guess you wanted to call ensureValidBoolExtendedAttribute and not ensureValidThrowsExtendedAttribute.
Or, hmm, ensureValidCanOOMExtendedAttribute isn't ever called. Remove it?

r- because it isn't quite clear what approach you were meant to take.

Comment 12

[Olli Pettay [:smaug]]

Comment on attachment 8826077 [details] [diff] [review]
part 3.  Add a subclass of FastErrorResult that can be used only to throw OOM

>+namespace dom {
>+namespace binding_detail {
>+class FastErrorResult :
>+    public mozilla::binding_danger::TErrorResult<
>+      mozilla::binding_danger::JustAssertCleanupPolicy>
>+{
>+};
>+} // namespace binding_detail
>+} // namespace dom
>+
>+// This part is a bit annoying.  We want an OOMReporter class that has the
>+// folowing properties:
following

>+class OOMReporter : private dom::binding_detail::FastErrorResult
>+{
>+public:
>+  void ReportOOM() {
nit, { goes to its own line

>+class OOMReporterInstantiator : public OOMReporter
>+{
>+public:
>+  OOMReporterInstantiator()
>+    : OOMReporter()
>+  {
>+  }
>+
>+  bool MaybeSetPendingException(JSContext* cx)
>+  {
>+    return OOMReporter::MaybeSetPendingException(cx);
Not yet clear to me why we need this method, but perhaps some latter patch explains it

Comment 13

[Olli Pettay [:smaug]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> and I have verified that isInfallible is false in canOOMAttr_getterInfo and
> canOOMAttr_setterinfo (because from the JS engine point of view they _are_
> fallible: they can throw OOM), but is true in canOOMSetterAttr_getterinfo.
What does http://searchfox.org/mozilla-central/rev/c477aa8bd99278962998adba1c5e4b15a02c42c7/js/src/jsfriendapi.h#2397-2398 mean? 'False in setters'?

Comment 14

[Olli Pettay [:smaug]]

Comment on attachment 8826083 [details] [diff] [review]
part 7.  Change nsINode::GetTextContent to take an OOMReporter, not an ErrorResult

Really nice. Very self-documenting code.

Comment 15

[Olli Pettay [:smaug]]

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #13)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> > and I have verified that isInfallible is false in canOOMAttr_getterInfo and
> > canOOMAttr_setterinfo (because from the JS engine point of view they _are_
> > fallible: they can throw OOM), but is true in canOOMSetterAttr_getterinfo.
> What does
> http://searchfox.org/mozilla-central/rev/
> c477aa8bd99278962998adba1c5e4b15a02c42c7/js/src/jsfriendapi.h#2397-2398
> mean? 'False in setters'?

oh, canOOMSetterAttr_getterinfo. it is _getterinfo

Comment 16

[Olli Pettay [:smaug]]

Comment on attachment 8826079 [details] [diff] [review]
part 5.  Correctly mark fallibility/movability/etc in jitinfo for cases that can OOM but are otherwise infallible

So getters are movable if infallible except with OOM:
movable = self.mayBeMovable() and getterinfal
that could use some comment why so.

I think eliminatable case is more understandable.

I'd prefer some jit person to take a look at this too.
perhaps jandem

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8826079 [details] [diff] [review]
part 5.  Correctly mark fallibility/movability/etc in jitinfo for cases that can OOM but are otherwise infallible

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

I'm not very familiar with Codegen.py, but moving getters/calls that can only throw on OOM (and don't have other observable side effects) out of loops should be fine.

(Just be careful we don't do this for getters/calls that are relatively expensive, as the optimization may backfire then. For instance if an expensive operation is in the loop but in a branch that's never taken, and we move it before the loop, we will end up doing more work than before.)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> so you pass "CanOOM" to ensureValidThrowsExtendedAttribute

Yikes, I really messed up this part of the patch.  Good catch!  It only matters for error reporting if the attribute is in fact invalid, which is why I didn't notice.  :(  What I _meant_ to write is this:

        def ensureValidThrowsExtendedAttribute(attr):
            ensureValidBoolExtendedAttribute(attr, "Throws")

        def ensureValidCanOOMExtendedAttribute(attr):
            ensureValidBoolExtendedAttribute(attr, "CanOOM")

and then maybeAppendCanOOMToAttrs calls ensureValidCanOOMExtendedAttribute.  Will attach an updated patch in a sec.

> following
> nit, { goes to its own line

Both fixed.

> Not yet clear to me why we need this method, but perhaps some latter patch explains it

It's there to make codegen simpler.  The idea is that codegen calls MaybeSetPendingException on its error-reporting thing, whether that's an ErrorResult or an OOMReporterInstantiator.

Codegen can't directly call OOMReporter::MaybeSetPendingException because that method lives on a private superclass of OOMReporter; the fact that OOMReporter is an ErrorResult thing under the hood is an implementation detail.  Hence this setup.  I'll add some comments.

> that could use some comment why so.

I'll add some comments like so:

            # At this point getterinfal is true if our getter either can't throw
            # at all, or can only throw OOM.  In both cases, it's safe to move,
            # or dead-code-eliminate, the getter, because throwing OOM is not
            # semantically meaningful, so code can't rely on it happening.  Note
            # that this makes the behavior consistent for OOM thrown from the
            # getter itself and OOM thrown from the to-JS conversion of the
            # return value (see the "canOOM" and "infallibleForMember" checks
            # below).

and similar for the method case.

> Just be careful we don't do this for getters/calls that are relatively expensive

That's the conundrum...  The .textContent getter _is_ somewhat expensive, which is why we want to allow it to be moved and eliminated.

That said, if the branch is literally _never_ taken, nbp tells me branch pruning will handle things (at least subject to the branch pruning heuristics): that code will simply not exist and hence not get loop-hoisted.

For the case of a branch that _has_ been taken in the past but isn't going to be taken in this invocation of the loop, you're right that we have a problem.  I _think_ this is a reasonable tradeoff, but we'll see...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 fixed to make sense

Comment 20

[Olli Pettay [:smaug]]

Comment on attachment 8827488 [details] [diff] [review]
Part 2 fixed to make sense

so getExtendedAttributes returns "CanOOM" for all the variants of OOM annotation, even though member.getExtendedAttribute is used for the actual annotation. Fine.
Perhaps worth to add a comment.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df802158c65c
part 1.  Add a CanOOM annotation, and the corresponding GetterCanOOM, SetterCanOOM annotations, to the IDL parser.  These can be used in the cases when Throws/GetterThrows/SetterThrows can be used, to indicate that the only possible exception is NS_ERROR_OUT_OF_MEMORY.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/555c3c62d3c0
part 2.  Change Descriptor.getExtendedAttributes to include 'canOOM' information as needed.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1b482c8b7e
part 3.  Add a subclass of FastErrorResult that can be used only to throw OOM.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b845082a91
part 4.  Pass OOMReporter from bindings in cases that can OOM but are otherwise infallible.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/067b7e8e4488
part 5.  Correctly mark fallibility/movability/etc in jitinfo for cases that can OOM but are otherwise infallible.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e94663d5fde0
part 6.  Add tests for code generation for CanOOM members.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7a5d9d060a
part 7.  Change nsINode::GetTextContent to take an OOMReporter, not an ErrorResult.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e67d594b1b
part 8.  Annotate Node.textContent as capable of OOM but not of throwing otherwise in the IDL.  r=smaug

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df802158c65c
https://hg.mozilla.org/mozilla-central/rev/555c3c62d3c0
https://hg.mozilla.org/mozilla-central/rev/ec1b482c8b7e
https://hg.mozilla.org/mozilla-central/rev/c9b845082a91
https://hg.mozilla.org/mozilla-central/rev/067b7e8e4488
https://hg.mozilla.org/mozilla-central/rev/e94663d5fde0
https://hg.mozilla.org/mozilla-central/rev/2f7a5d9d060a
https://hg.mozilla.org/mozilla-central/rev/30e67d594b1b