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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

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.

Comment 1

[Till Schneidereit [:till]]

Attachment: v1

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

Pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=3fec12d95e53

Comment 2

[Mozilla RelEng Bot]

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/[e-mail]-3fec12d95e53

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 657888 [details] [diff] [review]
v1

Could you put the 'options.selfHostingMode' check inside BytecodeEmitter::tellDebuggerAboutCompiledScript?

Comment 4

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cddd665d83f0

Thanks for the review!

Comment 5

[Ed Morley [:emorley]]

Backed out for failures in browser_dbg_bfcache.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed5024f9101

Comment 6

[Mozilla RelEng Bot]

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/[e-mail]-d923ae37897d

Comment 7

[Till Schneidereit [:till]]

Attachment: 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.

Comment 8

[Luke Wagner [:luke]]

I'm probably not able to evaluate this properly; I think jimb would be better.

Comment 9

[Till Schneidereit [:till]]

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

Over to jimb it goes, thanks

Comment 10

[Jim Blandy :jimb]

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

[Jim Blandy :jimb]

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.

Comment 12

[Jim Blandy :jimb]

... I missed one:

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

Comment 13

[Till Schneidereit [:till]]

Attachment: 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.

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-e3e80855ec3d

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking since this blocks another bug tracked for FF20 and we want this to land today for the coming beta build.

Comment 16

[Jim Blandy :jimb]

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!

Comment 17

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37dce17133d9

Comment 18

[Till Schneidereit [:till]]

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

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37dce17133d9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b062091ea4e
https://hg.mozilla.org/releases/mozilla-beta/rev/0ea8966da670

Comment 21

[Till Schneidereit [:till]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f166ec59f106

Comment 22

[Mihai Morar, (:MihaiMorar)]

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

Comment 23

[Till Schneidereit [:till]]

(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.

Comment 24

[Ed Morley [: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 :-)

Comment 25

[Mihai Morar, (:MihaiMorar)]

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!

Comment 26

[Mihai Morar, (:MihaiMorar)]

(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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for Fx21 due to bug 851788.
https://hg.mozilla.org/releases/mozilla-beta/rev/11275b673611