Bug 966665 - Don't DCE DOM method calls and getters that can throw exceptions. r=jandem, a=sledru
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 03 Feb 2014 09:55:52 -0500
changeset 182681 c1c9f264eccd46024df235ed7d00901f6604ae51
parent 182680 be285c8df31a814b5f882bdaf6a695d1ae19f9f1
child 182682 be8f5e5207318a11e872a0f8d434e624e2680166
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem, sledru
bugs966665
milestone29.0a2
Bug 966665 - Don't DCE DOM method calls and getters that can throw exceptions. r=jandem, a=sledru
dom/bindings/test/mochitest.ini
dom/bindings/test/test_throwing_method_noDCE.html
js/src/jit/MIR.cpp
js/src/jit/MIR.h
--- a/dom/bindings/test/mochitest.ini
+++ b/dom/bindings/test/mochitest.ini
@@ -27,10 +27,11 @@ support-files =
 [test_integers.html]
 [test_interfaceToString.html]
 [test_exceptions_from_jsimplemented.html]
 [test_lenientThis.html]
 [test_lookupGetter.html]
 [test_namedNoIndexed.html]
 [test_queryInterface.html]
 [test_sequence_wrapping.html]
+[test_throwing_method_noDCE.html]
 [test_treat_non_object_as_null.html]
 [test_traceProtos.html]
new file mode 100644
--- /dev/null
+++ b/dom/bindings/test/test_throwing_method_noDCE.html
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<title>Test that we don't DCE functions that can throw</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="log"></div>
+<script>
+test(function() {
+  function test(root) {
+    var threw = false;
+    try {
+        root.querySelectorAll("");
+    } catch(e){ threw = true; };
+    // Hot loop to make sure the JIT heuristics ion-compile this function even
+    // though it's throwing exceptions (which would normally make us back off
+    // of ion compilation).
+    for (var i=0; i<1500; i++) {}
+    return threw;
+  }
+
+  var threw = false;
+  var el = document.createElement("div");
+  for (var i=0; i<200; i++)
+      threw = test(el);
+  assert_true(threw);
+}, "Shouldn't optimize away throwing functions");
+</script>
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -667,20 +667,17 @@ MCall::New(TempAllocator &alloc, JSFunct
     if (!ins->init(alloc, maxArgc + NumNonArgumentOperands))
         return nullptr;
     return ins;
 }
 
 AliasSet
 MCallDOMNative::getAliasSet() const
 {
-    JS_ASSERT(getSingleTarget() && getSingleTarget()->isNative());
-
-    const JSJitInfo *jitInfo = getSingleTarget()->jitInfo();
-    JS_ASSERT(jitInfo);
+    const JSJitInfo *jitInfo = getJitInfo();
 
     JS_ASSERT(jitInfo->aliasSet() != JSJitInfo::AliasNone);
     // If we don't know anything about the types of our arguments, we have to
     // assume that type-coercions can have side-effects, so we need to alias
     // everything.
     if (jitInfo->aliasSet() != JSJitInfo::AliasDOMSets || !jitInfo->isTypedMethodJitInfo())
         return AliasSet::Store(AliasSet::Any);
 
@@ -718,20 +715,17 @@ MCallDOMNative::getAliasSet() const
 }
 
 void
 MCallDOMNative::computeMovable()
 {
     // We are movable if the jitinfo says we can be and if we're also not
     // effectful.  The jitinfo can't check for the latter, since it depends on
     // the types of our arguments.
-    JS_ASSERT(getSingleTarget() && getSingleTarget()->isNative());
-
-    const JSJitInfo *jitInfo = getSingleTarget()->jitInfo();
-    JS_ASSERT(jitInfo);
+    const JSJitInfo *jitInfo = getJitInfo();
 
     JS_ASSERT_IF(jitInfo->isMovable,
                  jitInfo->aliasSet() != JSJitInfo::AliasEverything);
 
     if (jitInfo->isMovable && !isEffectful())
         setMovable();
 }
 
@@ -765,16 +759,27 @@ MCallDOMNative::congruentTo(MDefinition 
         return false;
 
     // The other call had better be movable at this point!
     JS_ASSERT(call->isMovable());
 
     return true;
 }
 
+const JSJitInfo *
+MCallDOMNative::getJitInfo() const
+{
+    JS_ASSERT(getSingleTarget() && getSingleTarget()->isNative());
+
+    const JSJitInfo *jitInfo = getSingleTarget()->jitInfo();
+    JS_ASSERT(jitInfo);
+
+    return jitInfo;
+}
+
 MApplyArgs *
 MApplyArgs::New(TempAllocator &alloc, JSFunction *target, MDefinition *fun, MDefinition *argc,
                 MDefinition *self)
 {
     return new(alloc) MApplyArgs(target, fun, argc, self);
 }
 
 MDefinition*
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -1949,21 +1949,30 @@ class MCallDOMNative : public MCall
     // A helper class for MCalls for DOM natives.  Note that this is NOT
     // actually a separate MIR op from MCall, because all sorts of places use
     // isCall() to check for calls and all we really want is to overload a few
     // virtual things from MCall.
   protected:
     MCallDOMNative(JSFunction *target, uint32_t numActualArgs)
         : MCall(target, numActualArgs, false)
     {
+        // If our jitinfo is not marked movable, that means that our C++
+        // implementation is fallible or that we have no hope of ever doing the
+        // sort of argument analysis that would allow us to detemine that we're
+        // side-effect-free.  In the latter case we wouldn't get DCEd no matter
+        // what, but for the former case we have to explicitly say that we can't
+        // be DCEd.
+        if (!getJitInfo()->isMovable)
+            setGuard();
     }
 
     friend MCall *MCall::New(TempAllocator &alloc, JSFunction *target, size_t maxArgc,
                              size_t numActualArgs, bool construct, bool isDOMCall);
 
+    const JSJitInfo *getJitInfo() const;
   public:
     virtual AliasSet getAliasSet() const MOZ_OVERRIDE;
 
     virtual bool congruentTo(MDefinition *ins) const MOZ_OVERRIDE;
 
     virtual bool isCallDOMNative() const MOZ_OVERRIDE {
         return true;
     }
@@ -8027,16 +8036,21 @@ class MGetDOMProperty
 
         // Pin the guard as an operand if we want to hoist later
         setOperand(1, guard);
 
         // We are movable iff the jitinfo says we can be.
         if (isDomMovable()) {
             JS_ASSERT(jitinfo->aliasSet() != JSJitInfo::AliasEverything);
             setMovable();
+        } else {
+            // If we're not movable, that means we shouldn't be DCEd either,
+            // because we might throw an exception when called, and getting rid
+            // of that is observable.
+            setGuard();
         }
 
         setResultType(MIRType_Value);
     }
 
     const JSJitInfo *info() const {
         return info_;
     }