Closed Bug 1160182 Opened 6 years ago Closed 6 years ago

Handle extended functions in CreateLazyScriptsForCompartment

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Just noticed this; extended functions can also be relazified or have a LazyScript.
Attachment #8599900 - Flags: review?(shu)
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 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())
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 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.
Doh, I should've read those comments instead just go straight to the patch review.
Comment on attachment 8599900 [details] [diff] [review]
Patch

Clearing review for now.
Attachment #8599900 - Flags: review?(shu)
(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.
Flags: needinfo?(efaustbmo)
(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.
Flags: needinfo?(efaustbmo)
Attached patch PatchSplinter Review
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.
Attachment #8599900 - Attachment is obsolete: true
Attachment #8603237 - Flags: review?(shu)
(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.
(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 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.
Attachment #8603237 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/7a62238aecdc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Attachment #8603237 - Flags: approval-mozilla-esr38?
Attachment #8603237 - Flags: approval-mozilla-beta?
Attachment #8603237 - Flags: approval-mozilla-aurora?
Comment on attachment 8603237 [details] [diff] [review]
Patch

Approved for uplift to aurora (40) and beta (39)
Attachment #8603237 - Flags: approval-mozilla-beta?
Attachment #8603237 - Flags: approval-mozilla-beta+
Attachment #8603237 - Flags: approval-mozilla-aurora?
Attachment #8603237 - Flags: approval-mozilla-aurora+
> [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?
(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.
We are only backporting major/high impact security or stability fixes to ESR.
Comment on attachment 8603237 [details] [diff] [review]
Patch

Wontfix for 38esr since this is not a security/stability issue.
Attachment #8603237 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
This needs a little rebasing for beta.
Flags: needinfo?(jdemooij)
Thanks, rebased for beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/c0466694b30d
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.