966665 - Don't DCE DOM calls that can throw exceptions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: 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?

Comment 1

[Boris Zbarsky [: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?

Comment 2

[Boris Zbarsky [: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.

Comment 3

[Jan de Mooij [:jandem]]

(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?

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't DCE DOM method calls or attribute gets that can throw exceptions.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't DCE DOM method calls that can throw exceptions.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Test patch for testing attributes

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?

Comment 9

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 10

[Jan de Mooij [:jandem]]

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch for MGetDOMProperty

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up patch for both getters and methods

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05db27fe1164

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking, this won't land until post-merge.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05db27fe1164

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1c9f264eccd