bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Don't DCE DOM calls that can throw exceptions

RESOLVED FIXED in Firefox 29

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: bz)

Tracking

unspecified
mozilla30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 unaffected, firefox27 unaffected, firefox28 unaffected, firefox29+ fixed, firefox30 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8369106 [details]
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)
(Reporter)

Updated

5 years ago
status-firefox26: --- → unaffected
status-firefox27: --- → unaffected
status-firefox28: --- → unaffected
status-firefox29: --- → affected
tracking-firefox29: --- → ?
> 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.
(Reporter)

Comment 3

5 years ago
(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?
(Reporter)

Comment 4

5 years ago
(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.
Created attachment 8369148 [details] [diff] [review]
Don't DCE DOM method calls or attribute gets that can throw exceptions.
Attachment #8369148 - Flags: review?(jdemooij)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8369149 [details] [diff] [review]
Don't DCE DOM method calls that can throw exceptions.
Attachment #8369149 - Flags: review?(jdemooij)
Attachment #8369148 - Attachment is obsolete: true
Attachment #8369148 - Flags: review?(jdemooij)
Created attachment 8369150 [details] [diff] [review]
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?
Flags: needinfo?(jdemooij)
Whiteboard: [need review]
(Reporter)

Comment 9

5 years ago
Created attachment 8369345 [details]
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)
(Reporter)

Comment 10

5 years ago
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+
Created attachment 8369418 [details] [diff] [review]
Additional patch for MGetDOMProperty
Attachment #8369418 - Flags: review?(jdemooij)
Created attachment 8369420 [details] [diff] [review]
Roll-up patch for both getters and methods
Attachment #8369149 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
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.
tracking-firefox29: ? → +
https://hg.mozilla.org/mozilla-central/rev/05db27fe1164
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #8369420 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/c1c9f264eccd
status-firefox29: affected → fixed
status-firefox30: --- → fixed
You need to log in before you can comment on or make changes to this bug.