Closed Bug 610973 Opened 14 years ago Closed 14 years ago

Script error when extending String.prototype

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: Honza, Assigned: dmandelin)

References

Details

(Keywords: regression, Whiteboard: [firebug-p1][hardblocker][fixed-in-tracemonkey])

Attachments

(3 files, 2 obsolete files)

Chrome mochitest that passes
2.88 KB, patch
Details | Diff | Splinter Review
Test XPI 2
4.83 KB, application/x-xpinstall
Details
Patch
4.27 KB, patch
gal
: review+
Details | Diff | Splinter Review
Attached file Test XPI (obsolete) — 
This bug has been originally reported as Firebug issue 3632
http://code.google.com/p/fbug/issues/detail?id=3632

This problem happens when extending String.prototypes as follows:
String.prototype.foo = function() {}
"hello".foo();

The second line throws an error:
"hello".foo is not a function

Tested with:
http://hg.mozilla.org/mozilla-central/rev/4ef3abd2012c

STR:
1) Install attached extension (enables JSD and also creates a breakpoint on the first line for every created script)
2) Start Firefox and open Tools -> Error Console
2) Load this page: http://getfirebug.com/tests/issues/3632/issue3632.html
3) Follow instructions on the page (ie click the button)
4) See the error in the Error Console.

You might want to set some preferences to see the error in the console (see https://developer.mozilla.org/en/Setting_up_extension_development_environment)

Btw. I am also seeing:
Error: onError: hudIds is undefined, resource:///modules/HUDService.jsm, 2642
Source File: chrome://simpledebugger/content/debugger.js
Line: 117

Not sure if this is related. Tell me if I should create a separate report for it.

Honza
blocking2.0: --- → ?
Whiteboard: [firebug-p1]
This worksforme with a debug build from the changeset in comment 0 and the given steps to reproduce.
I didn't notice this before, but if I click three times on that button on the test page, I'll get the error.

Can you reproduce that now?
Honza
Ah, I did see a problem after three clicks, but in a debug build it hits a fatal assert due to the wrong String.prototype being used way before it gets to any sort of js error.  ;)  See my recent comments in bug 610941.
Here is yet another test-page that can be used to reproduce this bug without dealing with the error console.

1) Install attached extension (enables JSD and also creates a breakpoint on the
first line for every created script)
2) Load this page: http://getfirebug.com/tests/issues/3632/issue3632-1.html
3) The page should display "PASS", but it displays: "FAIL"

The source code of the page:

<body>FAIL</body>
<script>
String.prototype.foo = function() {}
"hello".foo();
document.body.textContent = "PASS";
</script>

Honza
The behavior in comment 4 appears to be by design. (!)

SimpleDebugger.BreakpointHook.onExecute returns undefined, which is the same as returning 0, which jsd interprets as "raise an uncatchable error". So basically, the addon in issue3632.xpi has the surprising effect of making *all* JS code loaded after it, both scripts and functions, instantly error out uncatchably.

To get the desired behavior, the onExecute method would have to return Ci.jsdIExecutionHook.RETURN_CONTINUE.
(In reply to comment #0)
> Btw. I am also seeing:
> Error: onError: hudIds is undefined, resource:///modules/HUDService.jsm, 2642
> Source File: chrome://simpledebugger/content/debugger.js
> Line: 117
> 
> Not sure if this is related. Tell me if I should create a separate report for
> it.

Probably bug 597136. It looks like there's a patch in the bug.
Here's the test I wrote to reproduce and investigate the issue.
(In reply to comment #5)
> The behavior in comment 4 appears to be by design. (!)
> 
> SimpleDebugger.BreakpointHook.onExecute returns undefined, 

So the test extension has a bug.

> To get the desired behavior, the onExecute method would have to return
> Ci.jsdIExecutionHook.RETURN_CONTINUE.

but Firebug issues RETURN_CONTINUE as far as I can tell. And this works in FF3.6.
But I think Jason is on to something. The test in :
http://getfirebug.com/tests/issues/3632/issue3632.html
passes, no error.

The test
http://getfirebug.com/tests/issues/3632/issue3632-1.html
fails. I think the reason here is that this case runs the code only at top-level, right when we are doing the funny business with BP=0
Just FYI, FF4.0 nightly is overwritting the extensions directory with .xpi files, which makes debugging with linking files very puzzling.
I verified that Firebug's onBreakpoint function is called and returns RETURN_CONTINUE for the test in 
http://getfirebug.com/tests/issues/3632/issue3632-1.html

I guess someone needs to verify that code path is taken on the inside.
I was just hit by this bug after updating FF beta 6 to beta 7. I'm using FireBug 1.7X.0a5. Disabling FireBug fixes the issue. Still wondering how FireBug can break plain JS features.
Attached file Test XPI 2 — 
The test:
http://getfirebug.com/tests/issues/3632/issue3632.html
passes for me since I have upgraded to:
http://hg.mozilla.org/mozilla-central/rev/0f17e5f1eb01

The test:
http://getfirebug.com/tests/issues/3632/issue3632-1.html
still fails

I am attaching fixed test extension.

Jason, sorry for the bug in the extension. Could you please retest yet once? There is definitely a bug somewhere. Thanks!

Honza
Attachment #489466 - Attachment is obsolete: true
methodjit/Compiler.cpp, line 3207:
    /* Bake in String.prototype. Is this safe? */

Apparently not. Debugging now.
JJB is totally right in comment 9. Tweaking the chrome mochitest to use an indirect eval makes it trigger the assertion reliably.

(In reply to comment #14)
> methodjit/Compiler.cpp, line 3207:
>     /* Bake in String.prototype. Is this safe? */
> 
> Apparently not. Debugging now.

I was wrong about this: it *is* safe, since we only get here if the script is compile-n-go. Remind me to remove the self-doubting part of the comment.

The real cause of the bug is, I think, either 

(1) that we call JS_ClearTrap with a cx and script that aren't in the same compartment; or

(2) something to do with the fp that Recompiler passes to Compiler.

I'm still thinking about how to fix this.
(In reply to comment #12)
> I was just hit by this bug after updating FF beta 6 to beta 7. I'm using
> FireBug 1.7X.0a5. Disabling FireBug fixes the issue. Still wondering how
> FireBug can break plain JS features.

There's no mystery: debuggers have backstage passes, they can not only see all the flats and cheats and tricks used by the VM to put on the "run JS efficiently yet correctly" show, and they can knock parts of the stage over, even during a live performance, by accident. This bug is one such show-spoiler.

/be
OS: Windows Vista → All
Hardware: x86 → All
Assignee: general → jorendorff
Just curious where this stands in the queue. I'm hoping that we'll have a decently happy Firebug soon. Any updates?
blocking2.0: ? → final+
Keywords: regression
Whiteboard: [firebug-p1] → [firebug-p1][hardblocker]
Blocks: 622708
Taking a look at this. In a debug build, with the test extension, I get a compartment mismatch assert trying to load the test case. Debugging now.
> The real cause of the bug is, I think, either 
> 
> (1) that we call JS_ClearTrap with a cx and script that aren't in the same
> compartment; or

We do that in jsd_ClearExecutionHook:

    JS_ClearTrap(jsdc->dumbContext, jsdscript->script, 
                 (jsbytecode*)pc, NULL, NULL );

Presumably jsd can't come up with a real context at this point. But it might be correct to set the compartment of the dumbContext to match the script. I'll have to see how dumbContext is set up and what that would mean.

> (2) something to do with the fp that Recompiler passes to Compiler.

The fp's compartment matches the script's compartment in this test case. That makes sense because the recompiler gets the fp that is active.
OK, I put in a cross-compartment call for JS_ClearTrap, but I still get 

  "hello".foo is not a function

Debugging more...
Assignee: jorendorff → dmandelin
Attached patch Patch (obsolete) — — Splinter Review 
There are actually 2 bugs here:

1. JSD passes a fake context to the recompiler, so when we get the string prototype with js_GetClassPrototype, we get the str proto for the fake context global. To get the right behavior, we must instead get it from the global that goes with the script.

2. There are some missing cross-compartment calls in jsd_scpt.c. 

I left one unfixed for now, in jsd_ClearAllExecutionHooksForScript, which calls JS_ClearScriptTraps. The reason is that this is called via the script destructor via GC. If we try to enter a cross-compartment call, we will find there is no script object, so we try to create one, but if we are out of memory, then we crash because we are already GC'ing. I think it is harmless to give the wrong call, because the script is dead and so we won't be recompiling it or doing anything much with it. Andreas, let me know if you think we should fix it, but I want to do that in a followup bug if so, so that Firebug users can start getting the right behavior for String.prototype.
Attachment #501892 - Flags: review?(gal)
Attachment #501892 - Flags: review?(dvander)
Comment on attachment 501892 [details] [diff] [review]
Patch

Patch needs a little cleanup. There is dead code and unnecessary changes in it.
Comment on attachment 501892 [details] [diff] [review]
Patch

r=me on methodjit changes
Attachment #501892 - Flags: review?(dvander) → review+
Attached patch Patch — — Splinter Review 

        Attachment #501892 -
        Attachment is obsolete: true

        Attachment #501892 -
        Flags: review?(gal)

    
  

        Attachment #501897 -
        Flags: review?(gal)

    
  

        Attachment #501897 -
        Flags: review?(gal) → review+

    
    

    
  
If you are sure that JS_ClearScriptTraps won't ever try to touch cx->compartment in any way, just take out there assert there with a comment.

    
    

    
  
(we already do this for JS_GetPrivate or so)

    
    

    
  
I'm landing in two parts, first methodjit change, then compartment fixes, because they are logically separate, and only the first is strictly required to fix this bug. That way we can back out independently if anything goes wrong. 

Part 1: methodjit fix:
http://hg.mozilla.org/tracemonkey/rev/1073e19109bd
Status: NEW → ASSIGNED
Whiteboard: [firebug-p1][hardblocker] → [firebug-p1][hardblocker][fixed-in-tracemonkey]

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/1073e19109bd

Resolving as fixed because of the migration to m-c -- dmandelin, please reopen and remove fixed-in-tracemonkey if you need to do the compartment fixes (per comment 29) in this bug.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
Separate bug for followup patching is best, as usual.

/be

    
    

    
  
(In reply to comment #30)
> http://hg.mozilla.org/mozilla-central/rev/1073e19109bd
> 
> Resolving as fixed because of the migration to m-c -- dmandelin, please reopen
> and remove fixed-in-tracemonkey if you need to do the compartment fixes (per
> comment 29) in this bug.

OK. At this point, the problem has been fixed. The compartment mismatch was an unrelated issue, although it looked like a possible cause originally. I don't get a compartment mismatch assert with a current debug build, and the recompiler "forgets" the original bogo-context, so I think for now the compartment assert fixes are not needed. They are still there in the patch (and easy to write anyway) if we need them later. So no action for now.

    
  
No longer blocks: 622708

    
    

    
  
I tested the user test case from firebug issue 3632 and it passes on tracemonkey  

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.14pre) Gecko/20110105 Namoroka/3.6.14pre

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: