Prevent self-hosted JS script from being registered with the debugger

RESOLVED FIXED in Firefox 20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: till, Assigned: till)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ verified, firefox21+ disabled, firefox22+ fixed)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
While we might consider showing self-hosted JS code in the debugger at some point, for now we should just hide the script altogether. We don't show the actual source code anyway, and we'll probably want to ponder the consequences of showing it a bit more in-depth.
(Assignee)

Comment 1

6 years ago
Created attachment 657888 [details] [diff] [review]
v1

This seems to work just fine and should catch all cases.

Pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=3fec12d95e53
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Attachment #657888 - Flags: review?(luke)

Comment 2

6 years ago
Try run for 3fec12d95e53 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3fec12d95e53
Results (out of 192 total builds):
    success: 173
    warnings: 16
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-3fec12d95e53

Comment 3

6 years ago
Comment on attachment 657888 [details] [diff] [review]
v1

Could you put the 'options.selfHostingMode' check inside BytecodeEmitter::tellDebuggerAboutCompiledScript?
Attachment #657888 - Flags: review?(luke) → review+

Updated

6 years ago
Depends on: 789117

Comment 5

6 years ago
Backed out for failures in browser_dbg_bfcache.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed5024f9101
Whiteboard: [js:t]

Updated

6 years ago
No longer depends on: 789117

Comment 6

5 years ago
Try run for d923ae37897d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d923ae37897d
Results (out of 98 total builds):
    exception: 1
    success: 84
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-d923ae37897d
(Assignee)

Comment 7

5 years ago
Created attachment 722876 [details] [diff] [review]
Prevent self-hosted JS script from being registered with the debugger

I revived this old patch because it fixes bug 844406.

There were quite a few more places where Debugger::onNewScript and js::CallNewScriptHook were called. Also, JS::DescribeStack has to be changed to leave out self-hosted scripts. That's the part that fixes bug 844406.

I'd very much like to get this into beta instead of backing out all self-hosted code.
Attachment #722876 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #657888 - Attachment is obsolete: true

Comment 8

5 years ago
I'm probably not able to evaluate this properly; I think jimb would be better.
(Assignee)

Comment 9

5 years ago
Comment on attachment 722876 [details] [diff] [review]
Prevent self-hosted JS script from being registered with the debugger

Over to jimb it goes, thanks
Attachment #722876 - Flags: review?(luke) → review?(jimb)
(Assignee)

Updated

5 years ago
Blocks: 844406

Comment 10

5 years ago
It's kind of inconsistent to not report self-hosted scripts via onNewScript, but to expose them via Debugger.prototype.findScripts, Debugger.Frame.prototype.script, and so on.

Could you at least change Debugger::ScriptQuery to skip them, as well?

Comment 11

5 years ago
Comment on attachment 722876 [details] [diff] [review]
Prevent self-hosted JS script from being registered with the debugger

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

This patch breaks a bunch of invariants the Debugger API would like to provide its users.

Each Debugger instance has a set of global objects that it considers its "debuggees". There are functions for adding globals to and removing globals from a particular Debugger's debuggee set.

Every script belongs to a particular compartment, which has a single associated global. If a script's compartment's global is a debuggee, we say that that script is a "debuggee script".

Variables are looked up in environments. If an environment's outermost enclosing environment is a debuggee global, that environment is a "debuggee environment".

A "debuggee function" or "debuggee frame" is one whose script and environment are debuggees.

An important invariant the Debugger API is supposed to maintain is that it only presents debuggee scripts/frames/environments, but that it presents all of them.

So the question we have to answer here is: Are self-hosted scripts ever debuggee scripts? It sounds like you want the answer to be "no, never". But if that's the decision, then it needs to be implemented consistently.

Debugger::observesFrame needs to check for self-hosted scripts, and declare frames running them to be non-observed. Then Debugger will never create Debugger.Frame instances for such frames to begin with.

DebuggerObject_getScript needs to check for functions whose scripts are self-hosted scripts, and return 'undefined', as it does for non-interpreted functions.

But I have a general question: what compartment do the self-hosted scripts live in? If all self-hosted scripts live in a compartment that's not shared with anyone else, then we could accomplish all the above more simply by just refusing to ever add the self-hosted scripts' compartment's global as a debuggee. Then the existing code to decide whether something is a debuggee X or not will just naturally apply, and self-hosted stuff will be trimmed out.

::: js/src/vm/Debugger.h
@@ +682,5 @@
>  void
>  Debugger::onNewScript(JSContext *cx, HandleScript script, GlobalObject *compileAndGoGlobal)
>  {
> +    if (script->selfHosted)
> +        return;

This check probably belongs in Debugger::slowPathOnNewScript; there's no need for it to be inlined everywhere.
Attachment #722876 - Flags: review?(jimb)

Comment 12

5 years ago
... I missed one:

Debugger::ScriptQuery::consider needs to skip self-hosted scripts as well.
(Assignee)

Comment 13

5 years ago
Created attachment 723101 [details] [diff] [review]
Prevent self-hosted JS script from being registered with the debugger

(In reply to Jim Blandy :jimb from comment #11)

Thanks for the quick review and the very helpful comments.

> So the question we have to answer here is: Are self-hosted scripts ever
> debuggee scripts? It sounds like you want the answer to be "no, never". But
> if that's the decision, then it needs to be implemented consistently.

That's exactly the decision, yes. (It probably makes sense to add the ability to make self-hosted scripts debuggable using an environment variable, but that's for another bug.)
 
> But I have a general question: what compartment do the self-hosted scripts
> live in? If all self-hosted scripts live in a compartment that's not shared
> with anyone else, then we could accomplish all the above more simply by just
> refusing to ever add the self-hosted scripts' compartment's global as a
> debuggee. Then the existing code to decide whether something is a debuggee X
> or not will just naturally apply, and self-hosted stuff will be trimmed out.

Self-hosted scripts are cloned into the compartments they are used in. Thus, censoring the self-hosting compartment won't work, unfortunately.

All other comments are addressed in the new patch.
Attachment #723101 - Flags: review?(jimb)
(Assignee)

Updated

5 years ago
Attachment #722876 - Attachment is obsolete: true

Comment 14

5 years ago
Try run for e3e80855ec3d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e3e80855ec3d
Results (out of 70 total builds):
    exception: 1
    success: 59
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-e3e80855ec3d
Tracking since this blocks another bug tracked for FF20 and we want this to land today for the coming beta build.
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
tracking-firefox20: --- → +
tracking-firefox21: --- → +
tracking-firefox22: --- → +

Comment 16

5 years ago
Comment on attachment 723101 [details] [diff] [review]
Prevent self-hosted JS script from being registered with the debugger

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

Looks great!
Attachment #723101 - Flags: review?(jimb) → review+
(Assignee)

Comment 18

5 years ago
Comment on attachment 723101 [details] [diff] [review]
Prevent self-hosted JS script from being registered with the debugger

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 784294
User impact if declined: Some extensions fail to work, especially GreaseMonkey. See bug 844406.
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #723101 - Flags: approval-mozilla-beta?
Attachment #723101 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/37dce17133d9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox22: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Attachment #723101 - Flags: approval-mozilla-beta?
Attachment #723101 - Flags: approval-mozilla-beta+
Attachment #723101 - Flags: approval-mozilla-aurora?
Attachment #723101 - Flags: approval-mozilla-aurora+
(In reply to Ed Morley [:edmorley UTC+0] from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/37dce17133d9

Ed, how can QA verify this fix?
Flags: needinfo?(emorley)
(Assignee)

Comment 23

5 years ago
(In reply to MarioMi (:MarioMi) from comment #22)
> Ed, how can QA verify this fix?

The following STR should work for verifying the fix:

1. Open the scratchpad and set its Environment to `Browser`
2. Enter the following script:

var frames = [];
function a(){
  let stack = Components.stack;
  do {
    frames.push(stack.filename);
  } while (stack = stack.caller);
  return true;
}
[1].forEach(function() {
	  a();
});
frames;

3. mark the script and chose `Display` from the Execute menu

Expected result:
The following string should be printed into the scratchpad:
/*
Scratchpad/1,Scratchpad/1,Scratchpad/1,
*/

Actual result without fix:
The following string is printed into the scratchpad:
/*
Scratchpad/1,Scratchpad/1,self-hosted,Scratchpad/1,
*/


@MarioMi: please request more info from me if you need anything more for the verification.
Flags: needinfo?(emorley)
(In reply to MarioMi (:MarioMi) from comment #22)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #19)
> > https://hg.mozilla.org/mozilla-central/rev/37dce17133d9
> 
> Ed, how can QA verify this fix?

When developers land on the mozilla-inbound tree, a sheriff then periodically mass-merges changesets across to mozilla-central - and uses a tool to mark the bugs with the changeset and close them. The best bet for working out who to ask for STR (since the sheriff won't have any context on the individual bugs), is the person in the assignee field, or else if that is empty, the author listed on the attached patches :-)
I confirm the fix is verified on FF 20.b5 on Windows 7 x64 and Mac OS 10.8

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0(20130313170052)


Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
(20130313170052)

Thanks Ed for info!
status-firefox20: fixed → verified

Updated

5 years ago
Depends on: 851788
(In reply to MarioMi (:MarioMi) from comment #25)

I confirm this is fixed on FF21b1 too: 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0

Builds ID: 20130401192816
status-firefox21: fixed → verified
Backed out for Fx21 due to bug 851788.
https://hg.mozilla.org/releases/mozilla-beta/rev/11275b673611
status-firefox21: verified → disabled
You need to log in before you can comment on or make changes to this bug.