Closed Bug 1319638 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
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)
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
hmm, Components is not defined in require.js... :/
Being off-by-one is fine by me.
Attachment #8813510 - Flags: feedback?(dteller) → feedback+
Blocks: 1317400
See Also: 1317400
maybe we could use eval instead.
I'll try implementing.
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)
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)
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+
Attachment #8841172 - Flags: review?(shu) → review+
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
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
https://hg.mozilla.org/mozilla-central/rev/21725697b47b
https://hg.mozilla.org/mozilla-central/rev/182d11502abe
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.