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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

5.93 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.11 KB, patch
jandem
: review+
smaug
: review+
Details | Diff | Splinter Review
5.89 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.00 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1000 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
3.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
Blocks: 1330475
jdm, ms2ger, servo may want something like this too.
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.
Attachment #8826081 - Flags: review?(bugs)
This compiles, which shows that the hackery to allow passing an ErrorResult as an OOMReporter is working.
Attachment #8826083 - Flags: review?(bugs)
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.
Attachment #8826084 - Flags: review?(bugs)
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.
Attachment #8826075 - Flags: review?(bugs) → review+
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.
Attachment #8826076 - Flags: review?(bugs) → review-
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
Attachment #8826077 - Flags: review?(bugs) → review+
Attachment #8826078 - Flags: review?(bugs) → review+
(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'?
Attachment #8826081 - Flags: review?(bugs) → review+
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.
Attachment #8826083 - Flags: review?(bugs) → review+
Attachment #8826084 - Flags: review?(bugs) → review+
(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 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
Attachment #8826079 - Flags: review?(jdemooij)
Attachment #8826079 - Flags: review?(bugs)
Attachment #8826079 - Flags: review+
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.)
Attachment #8826079 - Flags: review?(jdemooij) → review+
> 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...
Attachment #8826076 - Attachment is obsolete: true
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.
Attachment #8827488 - Flags: review?(bugs) → review+
Pushed by bzbarsky@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.