811584 - Handle lazy functions correctly in all cases in IM and JM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Attachment: v1

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 681317 [details] [diff] [review]
v1

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

Could you also add the test files that exercised the crash in JM/IM.  Given the immediate NULL-deref, they aren't security sensitive.

::: js/src/methodjit/Compiler.cpp
@@ +4436,5 @@
>  
>          /* Test if the function is scripted. */
>          stubcc.masm.load16(Address(icCalleeData, offsetof(JSFunction, flags)), tmp);
> +        Jump isNative = stubcc.masm.branchTest32(Assembler::NonZero, tmp,
> +                                                 Imm32(JSFunction::IS_LAZY));

Won't this (and the IM version) branch to the native path for a non-lazy interpreted function?

To use branchTest32, I think we'd want:
  INTERPRETED_WITH_SCRIPT = 0x1
  LAZY_INTERPRETED        = 0x2

and then define
  INTERPRETED_MASK = INTERPRETED_WITH_SCRIPT | LAZY_INTERPRETED

Comment 2

[Till Schneidereit [:till]]

Attachment: v2

Ugh, I should have known that I'm too tired to really test the patch, sorry.

This version does something slightly different to what you proposed, but to the same effect. Testing confirms the problem you mentioned and that this version fixes it.

Should I roll the patch into the one from bug 791850, now that that one has been backed out?

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 682070 [details] [diff] [review]
v2

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

Great!  r+, and yes it makes sense to fold this into the other patch.

1. I still don't see the test files attached to the patch.
2. Could you rename branchIfFunctionIsNative to branchIfFunctionDoesNotHaveScript?

::: js/src/jsfun.h
@@ +84,5 @@
>  
>      /* Possible attributes of an interpreted function: */
>      bool isHeavyweight()            const { return flags & HEAVYWEIGHT; }
>      bool isFunctionPrototype()      const { return flags & IS_FUN_PROTO; }
> +    bool isLazy()                   const { return flags & INTERPRETED_LAZY; }

Can this be isInterpretedLazy()?

Comment 4

[Till Schneidereit [:till]]

Fix has been merged into attachment 683832 [details] [diff] [review]