1160182 - Handle extended functions in CreateLazyScriptsForCompartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Just noticed this; extended functions can also be relazified or have a LazyScript.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The only extended functions that can be relazified are self-hosted ones, and those don't have a LazyScript.  Unless I'm missing something?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Not doing reviews right now from comment #1)
> The only extended functions that can be relazified are self-hosted ones, and
> those don't have a LazyScript.  Unless I'm missing something?

Arrow functions are extended and can be relazified AFAICS. The following condition in JSFunction::relazify is true for all non-selfhosted builtins + all self-hosted builtins that are extended:

(!isSelfHostedBuiltin() || isExtended())

Comment 3

[Jan de Mooij [:jandem]]

Or hm, maybe arrows don't have a LazyScript? Hm, will look into it more. If that's the case we should assert this in relazify() :)

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8599900 [details] [diff] [review]
Patch

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

Where is it that extended functions can have LazyScripts? I thought the only lazy extended functions were builtins, which aren't delazified via LazyScripts anyhow.

Comment 5

[Shu-yu Guo [:shu]]

Doh, I should've read those comments instead just go straight to the patch review.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8599900 [details] [diff] [review]
Patch

Clearing review for now.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Not doing reviews right now from comment #1)
> The only extended functions that can be relazified are self-hosted ones, and
> those don't have a LazyScript.  Unless I'm missing something?

(In reply to Shu-yu Guo [:shu] from comment #4)
> Where is it that extended functions can have LazyScripts?

I added an assert for this and it fails, because of.. methods! The getter `x` here is an extended function that can be relazified:

  {get x(){}}

Class methods at least are extended functions because they need to store the home object. I don't know if object literal methods also need this slot. efaust do you know?

Anyway, it looks like we should handle extended functions in CreateLazyScriptsForCompartment.

Comment 8

[Eric Faust [:efaust]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Class methods at least are extended functions because they need to store the
> home object. I don't know if object literal methods also need this slot.
> efaust do you know?
> 

object literal methods also store the home object, because super.prop also works there.

It's dumb. We should be able to tell at parse time whether we will need the slot (if we see super), and only allocate those as extended functions, but we have to allocate the function object before parsing.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

Also added a Debugger.findScripts testcase that fails without the patch.

Shu, at this point we might be able to revert bug 996982 and iterate over LazyScripts again. I can do that if you want, though personally I like that the current code also delazifies clones. It's probably a bit slower though.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #8)
> object literal methods also store the home object, because super.prop also
> works there.

Thanks, I changed the code to handle both normal functions and extended functions for now.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Created attachment 8603237 [details] [diff] [review]
> Patch
> 
> Also added a Debugger.findScripts testcase that fails without the patch.
> 
> Shu, at this point we might be able to revert bug 996982 and iterate over
> LazyScripts again. I can do that if you want, though personally I like that
> the current code also delazifies clones. It's probably a bit slower though.

I'm totally fine with the current code iterating functions.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8603237 [details] [diff] [review]
Patch

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

Thanks for the fix!

::: js/src/builtin/TestingFunctions.cpp
@@ +422,5 @@
>          return false;
>      }
>      if (!args[0].isObject() || !args[0].toObject().is<JSFunction>()) {
>          JS_ReportError(cx, "The first argument should be a function.");
> +        return false;

Oops.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a62238aecdc

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a62238aecdc

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8603237 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 996982.
[User impact if declined]: Debugger not seeing certain functions.
[Describe test coverage new/current, TreeHerder]: On m-c for a while.
[Risks and why]: None.
[String/UUID change made/needed]: None.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8603237 [details] [diff] [review]
Patch

Approved for uplift to aurora (40) and beta (39)

Comment 17

[Sylvestre Ledru [:Sylvestre]]

> [User impact if declined]: Debugger not seeing certain functions.
Can you explain what kind of functions you are talking about ? (to evaluate the fact to take it in esr38).

> [Risks and why]: None.
I don't think this evaluation is correct. Every patch has risk. Even one line changes...

Btw, Why did you request uplift for 38.0.5?

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Can you explain what kind of functions you are talking about ? (to evaluate
> the fact to take it in esr38).

Methods, which may become more widely used as people write more ES6:

{get foo() { return 3; }}

> Btw, Why did you request uplift for 38.0.5?

Because it affects ESR 38. If you don't think such correctness fixes have to be fixed on the ESR branch that's fine.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

We are only backporting major/high impact security or stability fixes to ESR.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8603237 [details] [diff] [review]
Patch

Wontfix for 38esr since this is not a security/stability issue.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b8b7ef36516

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This needs a little rebasing for beta.

Comment 23

[Jan de Mooij [:jandem]]

Thanks, rebased for beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/c0466694b30d