Closed
Bug 1330536
Opened 5 years ago
Closed 5 years ago
Add a way to annotate Web IDL things as throwing OOM only
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(8 files, 1 obsolete file)
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.
![]() |
Assignee | |
Comment 1•5 years ago
|
||
jdm, ms2ger, servo may want something like this too.
![]() |
Assignee | |
Comment 2•5 years ago
|
||
Attachment #8826075 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 3•5 years ago
|
||
Attachment #8826076 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 4•5 years ago
|
||
Attachment #8826077 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 5•5 years ago
|
||
Attachment #8826078 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 6•5 years ago
|
||
Attachment #8826079 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 7•5 years ago
|
||
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)
![]() |
Assignee | |
Comment 8•5 years ago
|
||
This compiles, which shows that the hackery to allow passing an ErrorResult as an OOMReporter is working.
Attachment #8826083 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 9•5 years ago
|
||
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)
![]() |
Assignee | |
Comment 10•5 years ago
|
||
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.
Updated•5 years ago
|
Attachment #8826075 -
Flags: review?(bugs) → review+
Comment 11•5 years ago
|
||
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 12•5 years ago
|
||
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+
Updated•5 years ago
|
Attachment #8826078 -
Flags: review?(bugs) → review+
Comment 13•5 years ago
|
||
(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'?
Updated•5 years ago
|
Attachment #8826081 -
Flags: review?(bugs) → review+
Comment 14•5 years ago
|
||
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+
Updated•5 years ago
|
Attachment #8826084 -
Flags: review?(bugs) → review+
Comment 15•5 years ago
|
||
(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•5 years ago
|
||
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 17•5 years ago
|
||
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+
![]() |
Assignee | |
Comment 18•5 years ago
|
||
> 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...
![]() |
Assignee | |
Comment 19•5 years ago
|
||
Attachment #8827488 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•5 years ago
|
Attachment #8826076 -
Attachment is obsolete: true
Comment 20•5 years ago
|
||
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+
Comment 21•5 years ago
|
||
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
Comment 22•5 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•