We currently throw a catchable exception when a concatenation would allocate a very large string. In this example, under Ion, the throw escapes the try block whereas on baseline it is caught as expected.

In this example, the MConcat is marked as movable, and LICM moves it outside the try block so exception is reported as firing from line 4. The naive fix would be to no longer mark as moveable, but we should measure the performance impact of this.

The second bug this uncovers is that we do not use a resumeAfter in |IonBuilder::binaryArithTryConcat| which causes |x| to have a stale value in the catch block.

Having a movable string concat seems like quite an useful optimization to me. Especially because we don't nursery allocate strings. In general we should be allowed to make this optimization, because internal errors are not specified, but the wrong line number is obviously confusing.

Ah, good point. This brings up another test case where we have dynamic guards around the concat. The guards would prevent the very large concat from executing. If it gets hoisted, we will throw.

Proposed solution for this is to use ConcatStrings<NoGC> and bailout if it fails. This also solves the problem of a MConcat that might OOM being hoisted from inside an if block that wouldn't execute at runtime.

Comment on attachment 8869287 [details] Bug 1365782 - Bailout from MConcat instead of throwing https://reviewboard.mozilla.org/r/140846/#review145120 Makes sense. ::: js/src/jit-test/tests/ion/bug1365782-1.js:11 (Diff revision 1) > + var x = 1; > + Array(1); > + x = 2; > + x = t + t; // May throw if too large > + } catch (e) { > + assertEq(x, 2); Maybe add a global variable var threw = 0; then here we can do threw++ and at the end of the file assertEq(threw, ..) ? ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js:1386 (Diff revision 1) > - rconcat_string(i); > - rconcat_number(i); > + // rconcat_string(i); // Disabled by Bug 1365782 > + // rconcat_number(i); // Disabled by Bug 1365782 This is a bit unfortunate but it's probably ok. We should check if it affects AWFY though. ::: js/src/jit/CodeGenerator.cpp:7451 (Diff revision 1) > masm.call(stringConcatStub); > - masm.branchTestPtr(Assembler::Zero, output, output, ool->entry()); > + masm.branchTestPtr(Assembler::NonZero, output, output, &done); > > + // If the concat would otherwise throw, we instead return nullptr and use > + // this to signal a bailout. This allows MConcat to be movable. > + masm.jmp(ool->entry()); s/jmp/jump, not sure if all platforms define jmp ::: js/src/jit/VMFunctions.cpp:1794 (Diff revision 1) > JSType type = js::TypeOfObject(obj); > return TypeName(type, *rt->commonNames); > } > > +bool > +ConcatStringsPure(JSContext* cx, JSString* lhs, JSString* rhs, MutableHandleString res) JSString** does not work I suppose? ::: js/src/jit/VMFunctions.cpp:1801 (Diff revision 1) > + // ConcatStrings without GC or throwing. If this fails, we set result to > + // nullptr and let caller do a bailout. Return true to indicate no exception > + // thrown. > + JSString* result = ConcatStrings<NoGC>(cx, lhs, rhs); > + res.set(result); > + return true; Can MOZ_ASSERT(!cx->isExceptionPending()); here.

Marking leave-open until we see AWFY impact.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/882d55c60444 Bailout from MConcat instead of throwing r=jandem

six-speed-templatestring-es6 is 10x slower by this. Need to reinvestigate. REOPENING.

Looks like this is another no-op benchmark that we were doing well at by DCE-ing the MConcat. Once we mark as setGuard to prevent different exception behavior from baseline, we slow down heavily.

After some discussion with :nbp and :jandem, it seems best to remove the setGuard. The InternalError exception is an artifact of our JString implementation and we shouldn't expect it be thrown if we can otherwise eliminate the operation. This will restore the six-speed perf, while still maintaining exceptions that do occur to appear in the correct location.

Fix AWFY regressions. Allow MConcat to be eliminated if result unused. The MConcat may throw on OOM or JString length overflow. These are both implementation details that are subject to change and shouldn't be expected to throw if we can avoid them. If they do throw, we ensure it happens where the user would expect it, even if the operation was moved by optimizations.

Backed out at Ted's request so it doesn't add extra risk to 55 going into Beta. https://hg.mozilla.org/integration/mozilla-inbound/rev/09084e1e008f1390659eced83bbea2b0a30644a7

This backout appears to have caused cgc tests to permafail: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&noautoclassify&fromchange=82ffae610a03a314544f54d6d79edafb3f0daa7f&filter-searchStr=709af0b1dfa7d4b188ae77714302b60db9a4ccf2&tochange=b693eb4b9ef6967c8a73e67e35df80705653ee2f&selectedJob=105554944 Any chance of a quick fix or should I backout the backout?

Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1e9ea66c7f Backout the backout of changeset 882d55c60444 because of cgc bustage because current bustage is worse than potential future risk a=bustage

also backedout from m-c https://hg.mozilla.org/mozilla-central/rev/09084e1e008f

Okay, I looked into the cgc failure. It looks like the failing test case is just too slow for cgc mode and should be added to cgc-jittest-timeouts. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea272d7d2ae49db2eb42052f714786f7f9c056c

Revert original patch and add test timeout exceptions. This returns us to original code state to avoid risk to upcoming beta55.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c26e124516f4 Backed out changeset 882d55c60444 to avoid adding risk to Beta55. https://hg.mozilla.org/integration/mozilla-inbound/rev/06ab93e49fa8 Disable jit-test/parser/bug-1355046.js on cgc for timeouts. r=test-only

Updating tracking. These patches have been reverted and original issue remains. The throw bug involves large strings allocations throwing an OOM where interpreter would otherwise not require an allocation. Marking existing versions as wontfix since there is no external indicators this affects people. Revisit for FF56 and see if there is a more stable fix, or if these implementation detail exceptions are not worth improving.

Target Milestone is used to track when a fix lands on m-c in general, so let's leave it un-set for now.

The leave-open keyword is there and there is no activity for 6 months. :tcampbell, maybe it's time to close this bug?

This was a mess that in the end was not worth landing. Nothing is "wrong" here, just might throw a catchable OOM sooner than expected. There was never any report of this in the wild.