Change line number of a function created by Function constructor to start from 1

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
In bug 755821, I kept the line numbering of function body, to make require() works like before
https://dxr.mozilla.org/mozilla-central/rev/0534254e9a40b4bade2577c631fe4cfa0b5db41d/toolkit/components/workerloader/require.js

but the comment says stack can be different, so if line number also can be different, that won't be the issue.

if there's no other critically affected things, we can change the line numbering so that function parameter starts from line 1.

it's required when the spec is changed to put 2 newlines before function body,
since now we're starting from line=0, to make function body starts from line 1,
but if there's 2 lines before body, this workaround is no longer applicable.
(Assignee)

Comment 1

a year ago
To be clear, even if we change the line numbering of Function constructor, we shouldn't change the line numbering of JS::CompipleFunction that is used by event handler.
or we should provide another API for event handler to make the function body starts from line 1.
(Assignee)

Comment 2

a year ago
Created attachment 8813510 [details] [diff] [review]
Change line number of a function created by Function constructor to start from 1.

So far, this hits only resource://gre/modules/workers/require.js, in tree.
(the change in devtools/client/inspector/markup/test/browser_markup_events3.js is just reverting a change by bug 755821)

Yoric, can I have your opinion about changing line number of error thrown by modules loaded by require.js ?
(since you implemented it)

for example:
  let { doThrow } = require(".../a.js");
  try {
    doThrow();                 // throws at line 10 in a.js
  } catch (e) {
    console.log(e.lineNumber); // logs 11
    console.log(e.stack);      // contains 11 instead of 10
  }

the difference between correct line and reported line can become 2 instead of 1, depending on the spec proposal.

If it's not acceptable, do you think introducing dedicated API that compiles module as a function makes sense?
something like

>         let code = Component.utils.compileFunction("exports", "require", "module",
>           source + "\n//# sourceURL=" + uri + "\n"
>         );
>         code(exports, require, module);

also, does resource://gre/modules/workers/require.js always have chrome privilege?
Attachment #8813510 - Flags: feedback?(dteller)
(Assignee)

Comment 3

a year ago
Created attachment 8813526 [details] [diff] [review]
(WIP) Part 2: Add Components.utils.compileFunction.

fwiw, here's draft patch that implements Components.utils.compileFunction.
it works like following (in xpcshell)

js> Components.utils.compileFunction("foo.js", ["a", "b", "c"], "return a + b + 
c");
function (a, b, c) {
return a + b + c
}

js> Components.utils.compileFunction("foo.js", [], "f()")();
JavaScript error: foo.js, line 1: ReferenceError: f is not defined
(Assignee)

Comment 4

a year ago
hmm, Components is not defined in require.js... :/
Being off-by-one is fine by me.
Attachment #8813510 - Flags: feedback?(dteller) → feedback+
(Assignee)

Updated

a year ago
Blocks: 1317400
(Assignee)

Updated

a year ago
See Also: bug 1317400
(Assignee)

Comment 6

9 months ago
maybe we could use eval instead.
I'll try implementing.
(Assignee)

Comment 7

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33f87336eb81a2e2f91b6188183d6542c07e267f
(Assignee)

Comment 8

9 months ago
Created attachment 8841171 [details] [diff] [review]
Part 1: Use Function+eval in require() in worker.

Okay, by using eval inside Function, we can keep the line numbering, even if the function body starts from line 2 or 3.
Attachment #8813510 - Attachment is obsolete: true
Attachment #8813526 - Attachment is obsolete: true
Attachment #8841171 - Flags: review?(dteller)
(Assignee)

Comment 9

9 months ago
Created attachment 8841172 [details] [diff] [review]
Part 2: Change line number of a function created by Function constructor to start from 1.

the line numbering issue in require() in Worker is solved by Part 1.

reverted the change done in bug 755821, that was to keep line numbering for function body for Function(...)
now function itself, and parameters starts from line 1, and body starts from line 3.

1: function(parameter here
2: ){
3: body here
4: }
Attachment #8841172 - Flags: review?(shu)
Comment on attachment 8841171 [details] [diff] [review]
Part 1: Use Function+eval in require() in worker.

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

::: toolkit/components/workerloader/require.js
@@ +136,5 @@
>          }
> +        // Use `Function` to leave this scope, use `eval` to start the line
> +        // number from 1 that is observed by `source` and the error message
> +        // thrown from the module, and also use `arguments` for accessing
> +        // `source` and `uri` to avoid polluting the module's environment.

I don't get it, how does using `arguments` change whether the environment is polluted? Also, are you using the backticks solely to avoid escaping double quotes?
Attachment #8841171 - Flags: review?(dteller)
(Assignee)

Comment 11

9 months ago
Thank you for reviewing :)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #10)
> Comment on attachment 8841171 [details] [diff] [review]
> Part 1: Use Function+eval in require() in worker.
> 
> Review of attachment 8841171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/workerloader/require.js
> @@ +136,5 @@
> >          }
> > +        // Use `Function` to leave this scope, use `eval` to start the line
> > +        // number from 1 that is observed by `source` and the error message
> > +        // thrown from the module, and also use `arguments` for accessing
> > +        // `source` and `uri` to avoid polluting the module's environment.
> 
> I don't get it, how does using `arguments` change whether the environment is
> polluted?

If I use formal parameter to receive `source` and `uri`, they're visible from the module code with those names.
With unpatched version, only `exports`, `require`, `module`, `arguments` are directly visible to module.
Of course other globals are visible too, but that's how this was working, and I think it's better to avoid
introducing extra variables to its scope.


> Also, are you using the backticks solely to avoid escaping double
> quotes?

I thought backticks or vertical bars are common for quoting variable names.
if double quotes are preferred here, I'll change them.
Comment on attachment 8841171 [details] [diff] [review]
Part 1: Use Function+eval in require() in worker.

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

got it!
Attachment #8841171 - Flags: review+

Updated

9 months ago
Attachment #8841172 - Flags: review?(shu) → review+
(Assignee)

Comment 13

9 months ago
thank you all for reviewing :)
Summary: Investigate the impact when we change the line numbering of a function created by Function constructor → Change line number of a function created by Function constructor to start from 1
(Assignee)

Comment 14

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/21725697b47bb567ea027e3f91faf333c76e43c8
Bug 1319638 - Part 1: Use Function+eval in require() in worker. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/182d11502abef1b63c6225f28d3629a04dfb9514
Bug 1319638 - Part 2: Change line number of a function created by Function constructor to start from 1. r=shu

Comment 15

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21725697b47b
https://hg.mozilla.org/mozilla-central/rev/182d11502abe
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.