Closed Bug 1565170 Opened 1 year ago Closed 11 months ago

Disable toSource/uneval in non-chrome code

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: devrel-note)

Attachments

(1 file)

This basically means toSource and uneval are going to be unavailable to normal web code. I think we can document this is as "normal" removal as well.

Before we can land this we first have to fix our tests.

Keywords: site-compat
Priority: -- → P1

uneval is used by the test function assertDeepEq to check if two symbols are equal. https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/js/src/tests/non262/shell.js#274

I don't think there is a standard way to implement this.

Depends on: 1565001
Assignee: evilpies → nobody

Tom, there's not a way to implement it that's guaranteed to last forever, but for the time being all the well-known symbols are defined as properties of Symbol... if that's what's blocking this bug, let me know and I'll patch assertDeepEq.

Otherwise ... bumping to P2. But there's no reason this can't happen.

Priority: P1 → P2
Assignee: nobody → evilpies

I am currently looking into disabling toSource/uneval in both non-chrome and chrome code.

Disabling toSource/uneval in chrome and content produces a ton of failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=073a0ddcb4f60bc776989389bae51a0be8a56c34
Same with disabling toSource/uneval just for content, maybe a bit less, but the test run isn't finished yet: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93918c6073b0127bdd9b8808ec03c1bf9b795994

I am going to start working on making the content-only case green.

There are various parts of the engine (especially error reporting) and Gecko (e.g. EventListenerInfo::ToSource) that use js::ValueToSource. For objects this function calls the toSource function to obtain the "source" representation. Without that methods the function falls back to just using ObjectToSource. Especially for the case where ValueToSource is called on function objects this results in a much worse results. I.e., "({})" instead of "(function() {})".

I think we can solve this by at least calling FunctionToSource for functions. We will also need to consider what to do with wrappers.

Depends on: 1605658
Depends on: 1605854
Attachment #9077337 - Attachment description: Bug 1565170 - Disable toSource/uneval in non-chrome code → Bug 1565170 - Disable toSource/uneval in non-chrome code.

I finally found the source of the test failure devtools/client/dom/test/browser_dom_array.js in bug 1605854.

The the current patch does not disable uneval properly. Because uneval is a global function (property of the global object), it is part of the "standard classes". A simple way of reproducing this problem is:

Object.getOwnPropertyNames(this).includes("uneval")

This should be false, but is true. uneval uses the JSProtoKey for String, so we can't even use GlobalObject::skipDeselectedConstructor to stop enumerating uneval in functions like EnumerateStandardClasses.

Depends on: 1606084
Attachment #9077337 - Attachment description: Bug 1565170 - Disable toSource/uneval in non-chrome code. → Bug 1565170 - Disable toSource/uneval in non-chrome code. r?jwalden r?bzbarsky
Attachment #9077337 - Attachment description: Bug 1565170 - Disable toSource/uneval in non-chrome code. r?jwalden r?bzbarsky → Bug 1565170 - Disable toSource/uneval in non-chrome code. r?jwalden,bzbarsky
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c67178218d1c
Disable toSource/uneval in non-chrome code. r=jwalden,bzbarsky
No longer depends on: 1606084
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Keywords: dev-doc-needed

Hey Philipp, this might be more prominent in Firefox extensions than on the web , probably worth notifying developers at least via the blog post, if not grepping code and emailing those potentially affected?

Flags: needinfo?(philipp)
Whiteboard: devrel-note

I'm sad toSource is going away, is this for security reasons? One thing I really liked about Firefox is being able to use toSource() to quickly inspect the object, using JSON.stringify requires resolving cyclic references and using devtools requires clicking a lot to expand the whole object. Maybe I'm missing some obvious alternatives though.

Andreas, can you grep and see how many developers are using this? If usage is low I think we could get away without a note about this.

Flags: needinfo?(philipp) → needinfo?(awagner)

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #12)

I'm sad toSource is going away, is this for security reasons? One thing I really liked about Firefox is being able to use toSource() to quickly inspect the object, using JSON.stringify requires resolving cyclic references and using devtools requires clicking a lot to expand the whole object. Maybe I'm missing some obvious alternatives though.

We are going to keep some of the code that was used by toSource, because it is used by various other internal code like error reporting. We simply should not expose non-standard methods to the web. I suggest you open a devtools bug to improve their object inspection, they could even directly expose JS_ValueToSource somehow.

Blocks: 1608734

I created Bug 1608734 for the devtools (as I'm using it a lot in the devtools console when diagnosing issues for webcompat).

Regressions: 1608668

2358 add-ons make use of uneval or toSource.

Flags: needinfo?(awagner)

This doesn't seem like a very high amount to me given the amount of submissions per quarter, but given a high profile add-on such as TST is affected I think it wouldn't hurt to reach out.

Documentation updates:

  • BCD PR 5735 has been submitted. This labels these two functions as removed from Firefox 74. I also added a note that explains that they're still there for privileged code, just because it will come up eventually. :)
  • Updated Firefox 74 for developers

fyi this created a regression on wunderlist.com
https://webcompat.com/issues/48331#issuecomment-590135362

(In reply to Karl Dubost💡 :karlcow from comment #18)

fyi this created a regression on wunderlist.com
https://webcompat.com/issues/48331#issuecomment-590135362

The piece of code which was failing:

  c = i.env.isFirefox() ? '(' + n.toSource() + ')()' : '(' + n.toString() + ')()';

and it was fixed with

  k = '(' + n.toString() + ')()';
Regressions: 1621574
Regressions: 1664725
You need to log in before you can comment on or make changes to this bug.