Closed Bug 966665 Opened 7 years ago Closed 7 years ago

Don't DCE DOM calls that can throw exceptions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: jandem, Assigned: bzbarsky)

References

Details

Attachments

(5 files, 2 obsolete files)

Attached file Testcase
The attached testcase fails for me with a recent Nightly. It looks like we DCE the root.querySelectorAll("") call and hence never throw/catch an exception.

bz/efaust, we probably want to call setGuard() on the MCall in this case, so that it's not eliminated. Do we know which DOM calls can throw exceptions and need this flag?
Flags: needinfo?(bzbarsky)
> Do we know which DOM calls can throw exceptions and need this flag?

We do.  The jitinfo has a boolean for this.  Unfortunately, that boolean also includes throwing due to OOM and such; once we decided we didn't need resume points for throwing DOM calls, I decided that we didn't need to separate out per-spec throwing (which is what we really care about here) and OOM throwing (which pretty much every single DOM call that's not returning a Number can do, since any gcthing allocation can throw).

We do mark such calls as not movable, because they can throw, but you're right that we need to explicitly prevent DCE, since the resume points aren't doing it for us anymore...  I'm sorry I didn't test this case carefully enough.  :(

Just to check, does setGuard() still allow other things that don't alias this method to be hoisted past it as needed?
Flags: needinfo?(bzbarsky)
So we could go ahead and prevent DCE of DOM calls that can throw for any reason, though that will be a bit annoying for cases like getAttribute() and getElementsByTagName() which can throw OOM but nothing else.

Or we could add another boolean to jitinfo to indicate "can throw for non-OOM reasons".  I think I'd somewhat prefer the latter.
(In reply to Boris Zbarsky [:bz] from comment #2)
> So we could go ahead and prevent DCE of DOM calls that can throw for any
> reason, though that will be a bit annoying for cases like getAttribute() and
> getElementsByTagName() which can throw OOM but nothing else.

getAttribute() will have jitinfo->isMovable, so it can't throw for non-OOM reasons, right?

Would it work to prevent DCE if !jitinfo->isInfallible && !jitinfo->isMovable?
(In reply to Boris Zbarsky [:bz] from comment #1)
> Just to check, does setGuard() still allow other things that don't alias
> this method to be hoisted past it as needed?

Forgot to answer this. Yes, setGuard() has no effect on LICM/GVN. All it does is prevent DCE of the instruction that has the flag set, nothing else.
> Would it work to prevent DCE if !jitinfo->isInfallible && !jitinfo->isMovable?

Yes, I like that.  In fact, jitinfo->isMovable exactly corresponds to "C++ implementation doesn't throw and is pure and we can try to analyze the arguments".  So simply !jitInfo->isMovable should be a sufficient test for preventing DCE.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8369149 - Flags: review?(jdemooij)
Attachment #8369148 - Attachment is obsolete: true
Attachment #8369148 - Flags: review?(jdemooij)
I tried to reproduce this with attribute getters.  We have no existing throwing pure attribute getters, so I just hacked one in.  But the testcase modified to call that getter (scrollTopMax) doesn't seem to fail.  Any idea why?
Flags: needinfo?(jdemooij)
Whiteboard: [need review]
Attached file Getter testcase
(In reply to Boris Zbarsky [:bz] from comment #8)
> But the testcase
> modified to call that getter (scrollTopMax) doesn't seem to fail.  Any idea
> why?

If the getter always throws we have an empty TypeSet and IonBuilder::jsop_getprop always takes a slow path in that case, see the types->empty() check. If I change GetScrollTopMax(ErrorResult& aRv) to:

    static int c = 0;
    if (c++ > 50)
      aRv.Throw(NS_ERROR_UNEXPECTED);

This test fails.
Flags: needinfo?(jdemooij)
Comment on attachment 8369149 [details] [diff] [review]
Don't DCE DOM method calls that can throw exceptions.

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

Looks good, thanks! Can you also fix MGetDOMProperty?
Attachment #8369149 - Flags: review?(jdemooij) → review+
Attachment #8369418 - Flags: review?(jdemooij)
Attachment #8369149 - Attachment is obsolete: true
Attachment #8369418 - Flags: review?(jdemooij) → review+
Comment on attachment 8369420 [details] [diff] [review]
Roll-up patch for both getters and methods

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 939581 
User impact if declined: Incorrect behavior on some sites, depending on JIT
   heuristics and whatnot.
Testing completed (on m-c, etc.): Passes attached tests.
Risk to taking this patch (and alternatives if risky): Low risk.  All the
   alternatives are worse.
String or IDL/UUID changes made by this patch: None.
Attachment #8369420 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/05db27fe1164
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla30
tracking, this won't land until post-merge.
https://hg.mozilla.org/mozilla-central/rev/05db27fe1164
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #8369420 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.