Handle extended functions in CreateLazyScriptsForCompartment

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

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

Firefox Tracking Flags

(firefox39 fixed, firefox40 fixed, firefox41 fixed, firefox-esr38 wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8599900 [details] [diff] [review]
Patch

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

Comment 2

3 years ago
(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())
(Assignee)

Comment 3

3 years ago
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

3 years ago
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

3 years ago
Doh, I should've read those comments instead just go straight to the patch review.
(Assignee)

Comment 6

3 years ago
Comment on attachment 8599900 [details] [diff] [review]
Patch

Clearing review for now.
Attachment #8599900 - Flags: review?(shu)
(Assignee)

Comment 7

3 years ago
(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)

Comment 8

3 years ago
(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)
(Assignee)

Comment 9

3 years ago
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.
Attachment #8599900 - Attachment is obsolete: true
Attachment #8603237 - Flags: review?(shu)
(Assignee)

Comment 10

3 years ago
(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

3 years ago
(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

3 years ago
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+

Comment 13

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a62238aecdc
https://hg.mozilla.org/mozilla-central/rev/7a62238aecdc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 15

3 years ago
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?
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox-esr38: --- → affected
(Assignee)

Comment 18

3 years ago
(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.
status-firefox-esr38: affected → wontfix
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-
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b8b7ef36516
status-firefox40: affected → fixed
Flags: in-testsuite+
This needs a little rebasing for beta.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 23

3 years ago
Thanks, rebased for beta:

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