Bug 40300 – Web Inspector: [JSC] implement script source editing

• Component: JavaScriptCore
• Status: NEW

[Yury Semikhatsky]

Should be possible to edit script sources on the fly.

[kangax]

This is now implemented, isn't it?

[Timothy Hatcher]

Not for JSC.

[Peter Wang]

Created attachment 148524 [details]
Patch

[WebKit Review Bot]

Attachment 148524 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 148524 [details]
Patch

Attachment 148524 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12995175

[Early Warning System Bot]

Comment on attachment 148524 [details]
Patch

Attachment 148524 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13003142

[Early Warning System Bot]

Comment on attachment 148524 [details]
Patch

Attachment 148524 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12995174

[Build Bot]

Comment on attachment 148524 [details]
Patch

Attachment 148524 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13009130

[Build Bot]

Comment on attachment 148524 [details]
Patch

Attachment 148524 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12997162

[Yury Semikhatsky]

Comment on attachment 148524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148524&action=review

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:91
> +    const ScriptValue& value = frame->script()->executeScript(newContent);

This will just re-evaluate the script as if you were running it in the inspector console while setScriptSource command should allow you to modify a function that may have already been run and leave other stuff untouched if its source doesn't change. E.g. changing 

"var count = 1;
function foo()
{
    count++;
}"

to

"var count = 1;
function foo()
{
    count--;
}"

shouldn't reset value of the count variable to 1.

[Peter Wang]

(In reply to comment #10)
> (From update of attachment 148524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148524&action=review
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:91
> > +    const ScriptValue& value = frame->script()->executeScript(newContent);
> 
> This will just re-evaluate the script as if you were running it in the inspector console while setScriptSource command should allow you to modify a function that may have already been run and leave other stuff untouched if its source doesn't change. E.g. changing 
> 
> "var count = 1;
> function foo()
> {
>     count++;
> }"
> 
> to
> 
> "var count = 1;
> function foo()
> {
>     count--;
> }"
> 
> shouldn't reset value of the count variable to 1.
I see... Thx.

[Peter Wang]

Created attachment 148768 [details]
Patch

[Build Bot]

Comment on attachment 148768 [details]
Patch

Attachment 148768 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13026083

[Build Bot]

Comment on attachment 148768 [details]
Patch

Attachment 148768 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13025089

[Peter Wang]

Created attachment 148778 [details]
Patch

[Build Bot]

Comment on attachment 148778 [details]
Patch

Attachment 148778 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13038077

[Build Bot]

Comment on attachment 148778 [details]
Patch

Attachment 148778 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13033086

[Early Warning System Bot]

Comment on attachment 148778 [details]
Patch

Attachment 148778 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13028122

[Early Warning System Bot]

Comment on attachment 148778 [details]
Patch

Attachment 148778 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13035089

[Gyuyoung Kim]

Comment on attachment 148778 [details]
Patch

Attachment 148778 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13027109

[WebKit Review Bot]

Comment on attachment 148778 [details]
Patch

Attachment 148778 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13025119

[Vsevolod Vlasov]

Comment on attachment 148778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148778&action=review

> Source/WebCore/bindings/js/PageScriptDebugServer.cpp:85
> +{

You should change the script with given sourceID live, you are evaluating newContent here, basically creating a new script instead of changing the old one.

[Peter Wang]

(In reply to comment #22)
> (From update of attachment 148778 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148778&action=review
> 
> > Source/WebCore/bindings/js/PageScriptDebugServer.cpp:85
> > +{
> 
> You should change the script with given sourceID live, you are evaluating newContent here, basically creating a new script instead of changing the old one.

I agree that your description is the ideal solution. But it seems impossible for JSC, since neither JSC nor Frame will cache the JS code they executed. JSC just read the string of JS code, "compile" it into Identifiers and executable code blocks, save these things in Global Data structure, and execute. 

My solution is taking an advantage of JSC: when JSC is "compiling" JS code, if there is an Identifier with an existent name in Global Data, JSC will replace the old with the new.

So I did a little modification of "Interpreter::execute": if we just need to recompile, we send a flag to it to let it just replace the old Identifiers and executable code blocks but don't really execute them. By that way we implement the effect of "editing live js code".

[Peter Wang]

Created attachment 149225 [details]
Patch

[Early Warning System Bot]

Comment on attachment 149225 [details]
Patch

Attachment 149225 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13026992

[Early Warning System Bot]

Comment on attachment 149225 [details]
Patch

Attachment 149225 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13094006

[Build Bot]

Comment on attachment 149225 [details]
Patch

Attachment 149225 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13090023

[Build Bot]

Comment on attachment 149225 [details]
Patch

Attachment 149225 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13087071

[Gyuyoung Kim]

Comment on attachment 149225 [details]
Patch

Attachment 149225 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13089037

[Peter Wang]

Created attachment 149240 [details]
Patch

[Build Bot]

Comment on attachment 149240 [details]
Patch

Attachment 149240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13098014

[Peter Wang]

Created attachment 149259 [details]
Patch

[Build Bot]

Comment on attachment 149259 [details]
Patch

Attachment 149259 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13090081

[Peter Wang]

Created attachment 149263 [details]
Patch

[Vsevolod Vlasov]

Someone who knows JSC better should take a look on this.

[Geoffrey Garen]

<rdar://problem/11741197>

[Peter Wang]

The main modifications of this patch are:
(1) Add a flag to JSC::Interpret::execute, if this flag is true that means we want to compile the code only. 
(2) In PageScriptDebugServer, record the map between frame and sourceID of JS to ensure the modified JS code will be recompiled in same world. 
(3) Implement WebCore::ScriptDebugServer::setScriptSource by calling JSC::Interpret::execute with a flag.


This solution takes the advantage that when JSC compile the same script it will replace the old Identifier with the new one.

[Yong Li]

Comment on attachment 149263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149263&action=review

> Source/JavaScriptCore/ChangeLog:8
> [phone]  Peter Wang  <[e-mail]>
> +
> +        [JSC] Web Inspector: implement script source editing
> +        https://bugs.webkit.org/show_bug.cgi?id=40300
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * debugger/DebuggerCallFrame.cpp:

Can you add more details in changelog?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool justRecompile)

I believe "fooOnly" is more popular pattern in webkit.

> Source/JavaScriptCore/runtime/Completion.h:37
> +    JS_EXPORT_PRIVATE JSValue evaluate(ExecState*, ScopeChainNode*, const SourceCode&, bool, JSValue thisValue = JSValue(), JSValue* exception = 0);
>  } // namespace JSC

Why isn't justRecompile the last argument here?

> Source/WebCore/bindings/js/PageScriptDebugServer.h:58
> +    typedef HashMap<String, Frame*> SourceIdToGlobalObjectMap;

should be SourceIDToGlobalObjectMap?

Are we sure using Frame* is always safe? I.E. no frame can be freed during the life time of this map?

Also, is it possible that one script (or one script ID) can be shared by multiple frames?

> Source/WebCore/bindings/js/ScriptController.cpp:130
> +
> +    RefPtr<Frame> protect(m_frame);

why do we need this? I don't see how recompiling a script can free the Frame object.

> Source/WebCore/bindings/js/ScriptController.h:89
> +    JSC::ExecState* getScriptState();

WebKit doesn't use "getFoo" to name such a getter that passes result only through return value. it could be mainThreadNormalWorldState() -- probably too long? :)

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:201
>  bool ScriptDebugServer::canSetScriptSource()
>  {
> -    return false;
> +    return true;
>  }

why return true here?

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:206
>  bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*)
>  {
> -    // FIXME(40300): implement this.
> -    return false;
> +    return true;
>  }

why return true here but nothing is done?

[Yong Li]

Comment on attachment 149263 [details]
Patch

r- for lack of detailed descriptions in changelog. Also, can you provide a test case (even manual one) or mention how others can test it?

[Peter Wang]

(In reply to comment #38)
> (From update of attachment 149263 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149263&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > [phone]  Peter Wang  <[e-mail]>
> > +
> > +        [JSC] Web Inspector: implement script source editing
> > +        https://bugs.webkit.org/show_bug.cgi?id=40300
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * debugger/DebuggerCallFrame.cpp:
> 
> Can you add more details in changelog?
ok. 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool justRecompile)
> 
> I believe "fooOnly" is more popular pattern in webkit.
ok, thx. 
> > Source/JavaScriptCore/runtime/Completion.h:37
> > +    JS_EXPORT_PRIVATE JSValue evaluate(ExecState*, ScopeChainNode*, const SourceCode&, bool, JSValue thisValue = JSValue(), JSValue* exception = 0);
> >  } // namespace JSC
> 
> Why isn't justRecompile the last argument here?
Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. 
> > Source/WebCore/bindings/js/PageScriptDebugServer.h:58
> > +    typedef HashMap<String, Frame*> SourceIdToGlobalObjectMap;
> 
> should be SourceIDToGlobalObjectMap?
sure, thx. 
> Are we sure using Frame* is always safe? I.E. no frame can be freed during the life time of this map?
Yes, if the frame is freed the inspector will refresh. So if you can see the source code in inspector, the related frame must exists.
> Also, is it possible that one script (or one script ID) can be shared by multiple frames?
Never. Actually the ID here is the address of SourceProvider of a JS code block. We take it as a index to record from which frame this piece of JS code is executed.  
> > Source/WebCore/bindings/js/ScriptController.cpp:130
> > +
> > +    RefPtr<Frame> protect(m_frame);
> 
> why do we need this? I don't see how recompiling a script can free the Frame object.
Yes, thx. It's the code from my old patch. It's useless now and should be removed.
> > Source/WebCore/bindings/js/ScriptController.h:89
> > +    JSC::ExecState* getScriptState();
> 
> WebKit doesn't use "getFoo" to name such a getter that passes result only through return value. it could be mainThreadNormalWorldState() -- probably too long? :)
ok. 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201
> >  bool ScriptDebugServer::canSetScriptSource()
> >  {
> > -    return false;
> > +    return true;
> >  }
> 
> why return true here?
It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report.     
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206
> >  bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*)
> >  {
> > -    // FIXME(40300): implement this.
> > -    return false;
> > +    return true;
> >  }
> 
> why return true here but nothing is done?
This function is never be invoked. Its override in PageScriptDebugServer will play the rule. For JSC we cannot implement the feature at "ScriptDebugServer" level as V8 does, since V8 seems have a extra shell layer to record the origin of JS source code.

[Peter Wang]

Created attachment 150884 [details]
Patch

[Build Bot]

Comment on attachment 150884 [details]
Patch

Attachment 150884 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13145031

[Peter Wang]

Created attachment 150887 [details]
Patch

[Yong Li]

(In reply to comment #40)
> (In reply to comment #38)
> > Why isn't justRecompile the last argument here?
> Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. 

Then we can give a default value? or move arg to the similar position for other functions? It is better to see consistent order.

> > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201
> > >  bool ScriptDebugServer::canSetScriptSource()
> > >  {
> > > -    return false;
> > > +    return true;
> > >  }
> > 
> > why return true here?
> It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report.

I see we are using PageScriptDebugServer. Can we make it virtual and leave the base class to return false?

> > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206
> > >  bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*)
> > >  {
> > > -    // FIXME(40300): implement this.
> > > -    return false;
> > > +    return true;
> > >  }
> > 
> > why return true here but nothing is done?
> This function is never be invoked. Its override in PageScriptDebugServer will play the rule. For JSC we cannot implement the feature at "ScriptDebugServer" level as V8 does, since V8 seems have a extra shell layer to record the origin of JS source code.

If it is never invoked, why do we change it? A pure virtual function or ASSERT_NOT_REACHED may be better.

> > 
> > should be SourceIDToGlobalObjectMap?
> sure, thx. 

m_sourceIdToGlobalObjectMap is still there :)

[Peter Wang]

(In reply to comment #44)
> (In reply to comment #40)
> > (In reply to comment #38)
> > > Why isn't justRecompile the last argument here?
> > Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. 
> 
> Then we can give a default value? or move arg to the similar position for other functions? It is better to see consistent order.

Yes, but there is a trivial reason causes we cannot do that. Firstly, we have to keep the old interface untouched:
evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0);
Otherwise we will break the windows porting since it's a interface of DLL.
Then if we put "compileOnly" in last position with a default value, looks like this: 
evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0, bool compileOnly = false);
The compiler will confused it with the old interface and complain. 
> > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201
> > > >  bool ScriptDebugServer::canSetScriptSource()
> > > >  {
> > > > -    return false;
> > > > +    return true;
> > > >  }
> > > 
> > > why return true here?
> > It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report.
> 
> I see we are using PageScriptDebugServer. Can we make it virtual and leave the base class to return false?
Don't worry this function too much. It will be removed soon after this bug fixed, like what we did in bug#88759 
> > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:206
> > > >  bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*)
> > > >  {
> > > > -    // FIXME(40300): implement this.
> > > > -    return false;
> > > > +    return true;
> > > >  }
> > > 
> > > why return true here but nothing is done?
> > This function is never be invoked. Its override in PageScriptDebugServer will play the rule. For JSC we cannot implement the feature at "ScriptDebugServer" level as V8 does, since V8 seems have a extra shell layer to record the origin of JS source code.
> 
> If it is never invoked, why do we change it? A pure virtual function or ASSERT_NOT_REACHED may be better.
Agree with ASSERT_NOT_REACHED, it tells design intension more clearly. Thx.  
> > > 
> > > should be SourceIDToGlobalObjectMap?
> > sure, thx. 
> 
> m_sourceIdToGlobalObjectMap is still there :)
I'm so sorry. It'll be corrected.

[Peter Wang]

Created attachment 151015 [details]
Patch

[Yong Li]

(In reply to comment #45)
> (In reply to comment #44)
> > (In reply to comment #40)
> > > (In reply to comment #38)
> > > > Why isn't justRecompile the last argument here?
> > > Because the last two parameters have default value, if we pull justRecompile in last position we have to set a default value otherwise it causes a compiling error. 
> > 
> > Then we can give a default value? or move arg to the similar position for other functions? It is better to see consistent order.
> 
> Yes, but there is a trivial reason causes we cannot do that. Firstly, we have to keep the old interface untouched:
> evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0);
> Otherwise we will break the windows porting since it's a interface of DLL.
> Then if we put "compileOnly" in last position with a default value, looks like this: 
> evaluate(ExecState*, ScopeChainNode*, const SourceCode&, JSValue thisValue = JSValue(), JSValue* exception = 0, bool compileOnly = false);
> The compiler will confused it with the old interface and complain. 

Can't we change the DLL interface or ask Apple gentlemen for help?

> > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:201
> > > > >  bool ScriptDebugServer::canSetScriptSource()
> > > > >  {
> > > > > -    return false;
> > > > > +    return true;
> > > > >  }
> > > > 
> > > > why return true here?
> > > It's a legacy code, since JSC doesn't support this feature but V8 support. Inpsector uses this interface to probe it. If this bug is fixed, this function plus related JS code of front-end will be removed in a dependent bug report.
> > 
> > I see we are using PageScriptDebugServer. Can we make it virtual and leave the base class to return false?
> Don't worry this function too much. It will be removed soon after this bug fixed, like what we did in bug#88759 

We shouldn't push unfinished thing into the repo. Logically ScriptDebugServer doesn't support setScriptSource() by itself.

[Konrad Piascik]

Comment on attachment 151015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review

> Source/WebCore/ChangeLog:12
> +        The related test cases already exist.

Do the tests need to be enalbed on JSC builds? ie unskipped?

[Yong Li]

Comment on attachment 151015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:207
>  bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*)
>  {
> -    // FIXME(40300): implement this.
> +    ASSERT_NOT_REACHED();
>      return false;
>  }

As pointed by Konrad, ASSERT_NOT_REACHED() looks weird here because pure virtual function makes more sense. sorry for my old suggestion.

[Yong Li]

Can JSC experts put some comments? Is it the right approach?

[Peter Wang]

(In reply to comment #48)
> (From update of attachment 151015 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        The related test cases already exist.
> 
> Do the tests need to be enalbed on JSC builds? ie unskipped?

Frankly speaking, I don't plan to unskip related test case here, since at least Qt porting still has some problems to support some inspector test, like bug#89056. I'm planning to enable it in an independent bug report to make it more clear.

[Peter Wang]

(In reply to comment #49)
> (From update of attachment 151015 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151015&action=review
> 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:207
> >  bool ScriptDebugServer::setScriptSource(const String&, const String&, bool, String*, ScriptValue*, ScriptObject*)
> >  {
> > -    // FIXME(40300): implement this.
> > +    ASSERT_NOT_REACHED();
> >      return false;
> >  }
> 
> As pointed by Konrad, ASSERT_NOT_REACHED() looks weird here because pure virtual function makes more sense. sorry for my old suggestion.

I agree with Konrad that pure virtual function is better, but some porting derived their own new class form "ScriptDebugServer", pure virtual function will break their own code :(.

[Peter Wang]

Created attachment 151849 [details]
Patch

[Peter Wang]

(In reply to comment #53)
> Created an attachment (id=151849) [details]
> Patch

Since the interface "setScriptSource" for JSC is implemented in "PageScriptDebugServer", so set it as pure virtual function to avoid misuse and make the meaning of code more clear.

[Yong Li]

So, we don't worry about this any more? "some porting derived their own new class form "ScriptDebugServer"

Also ScriptDebugServer::canSetScriptSource() should be virtual. It is weird that the base class forces all derived classes to support it. If so, why do we even call this method?

Back to the approach you are doing, have you tested the case that the script has been referenced by other scripts? I am afraid some JIT assemblies can still be using the old executable, or even worse, linking to freed memory.

[Filip Pizlo]

Comment on attachment 151849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)

It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().

> Source/JavaScriptCore/runtime/Completion.cpp:-56
> -    JSLockHolder lock(exec);

Did you put in some locking elsewhere to justify the removal of this lock acquisition?

[Peter Wang]

(In reply to comment #55)
> So, we don't worry about this any more? "some porting derived their own new class form "ScriptDebugServer"
> 
> Also ScriptDebugServer::canSetScriptSource() should be virtual. It is weird that the base class forces all derived classes to support it. If so, why do we even call this method?

Like bug#88759, this bug removed an interface ScriptDebugServer::supportsNativeBreakpoints(), this interface existed just because JSC didn't support a feature that V8 supports. As JSC started to support it, this interface became useless and was removed.
We'll do the same thing to canSetScriptSource(). 
These interfaces are just transitions, so eventually they will be removed.    

 
> Back to the approach you are doing, have you tested the case that the script has been referenced by other scripts? I am afraid some JIT assemblies can still be using the old executable, or even worse, linking to freed memory.
Yes, I did. 
My approach is almost executing the same piece of JS again, just skip the step of creating a new call frame and reading the executable to evaluate. All the operations that can change the executable are still included, so I think it's safe.

[Peter Wang]

(In reply to comment #56)
> (From update of attachment 151849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)
> 
> It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().

I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly.
It's the reason.  

> 
> > Source/JavaScriptCore/runtime/Completion.cpp:-56
> > -    JSLockHolder lock(exec);
> 
> Did you put in some locking elsewhere to justify the removal of this lock acquisition?

Oops, sorry. It's supposed to be here. It's a mistake, seems imported from attachment 149263 [details]. I'll correct it.

[Peter Wang]

Created attachment 152134 [details]
Patch

[Filip Pizlo]

Comment on attachment 152134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152134&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)

Split this into two methods, one that compiles, and the other that compiles and executes.

[Filip Pizlo]

(In reply to comment #58)
> (In reply to comment #56)
> > (From update of attachment 151849 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> > 
> > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)
> > 
> > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().
> 
> I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly.
> It's the reason.  

Are you joking?

> 
> > 
> > > Source/JavaScriptCore/runtime/Completion.cpp:-56
> > > -    JSLockHolder lock(exec);
> > 
> > Did you put in some locking elsewhere to justify the removal of this lock acquisition?
> 
> Oops, sorry. It's supposed to be here. It's a mistake, seems imported from attachment 149263 [details]. I'll correct it.

[Peter Wang]

(In reply to comment #61)
> (In reply to comment #58)
> > (In reply to comment #56)
> > > (From update of attachment 151849 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> > > 
> > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)
> > > 
> > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().
> > 
> > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly.
> > It's the reason.  
> 
> Are you joking?
> 
?? 
No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling.

[Filip Pizlo]

(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #58)
> > > (In reply to comment #56)
> > > > (From update of attachment 151849 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> > > > 
> > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)
> > > > 
> > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().
> > > 
> > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly.
> > > It's the reason.  
> > 
> > Are you joking?
> > 
> ?? 
> No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling.

Invoking a method called "execute" in order to compile something is bizarre.

[Peter Wang]

(In reply to comment #63)
> (In reply to comment #62)
> > (In reply to comment #61)
> > > (In reply to comment #58)
> > > > (In reply to comment #56)
> > > > > (From update of attachment 151849 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> > > > > 
> > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> > > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)
> > > > > 
> > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().
> > > > 
> > > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly.
> > > > It's the reason.  
> > > 
> > > Are you joking?
> > > 
> > ?? 
> > No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling.
> 
> Invoking a method called "execute" in order to compile something is bizarre.

Yes, so I'd like to discuss. Based on the current situation, by my opinion, I think there are three ways to solve it:
(1) Have independent compile() and execute() with a big block of same code.
(2) Have execute() with a parameter.
(3) Move compiling related code to an independent function compile(), and let execute() to invoke it.    
I said the reason why I don't choose (1). I prefer (2) because it brings minimal modification. Do you mean you prefer (3)? I just concern there is too big modification.

[Filip Pizlo]

(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > (In reply to comment #61)
> > > > (In reply to comment #58)
> > > > > (In reply to comment #56)
> > > > > > (From update of attachment 151849 [details] [details] [details] [details] [details] [details])
> > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> > > > > > 
> > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094
> > > > > > > -JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj)
> > > > > > > +JSValue Interpreter::execute(ProgramExecutable* program, CallFrame* callFrame, ScopeChainNode* scopeChain, JSObject* thisObj, bool compileOnly)
> > > > > > 
> > > > > > It would be better if we created a compile() helper, preferably outside of the Interpreter class if it's easy.  That way instead of saying execute(blah, true) you'd say compile().
> > > > > 
> > > > > I considered this way once. But Interpreter::execute implements all steps of compiling, so that if I want to write an independent compile(), I just need to copy Interpreter::execute and delete several lines, it looks ugly.
> > > > > It's the reason.  
> > > > 
> > > > Are you joking?
> > > > 
> > > ?? 
> > > No. I'm serious. Parsing, Building AST, Generiting bytecode, creating call frame to evaluate the executable. Except the last step, Interpreter::execute does all steps of compiling.
> > 
> > Invoking a method called "execute" in order to compile something is bizarre.
> 
> Yes, so I'd like to discuss. Based on the current situation, by my opinion, I think there are three ways to solve it:
> (1) Have independent compile() and execute() with a big block of same code.

Bad.

> (2) Have execute() with a parameter.

Bad.

> (3) Move compiling related code to an independent function compile(), and let execute() to invoke it.    

That's what I'm asking for.

> I said the reason why I don't choose (1). I prefer (2) because it brings minimal modification. Do you mean you prefer (3)? I just concern there is too big modification.

(3) is the only sensible path.

[Peter Wang]

(In reply to comment #65)
> (In reply to comment #64)
> > (In reply to comment #63)
> > > (In reply to comment #62)
> > > > (In reply to comment #61)
> > > > > (In reply to comment #58)
> > > > > > (In reply to comment #56)
> > > > > > > (From update of attachment 151849 [details] [details] [details] [details] [details] [details] [details])
> > > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=151849&action=review
> > > > > > > 
> > > > > > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:1094

> > (3) Move compiling related code to an independent function compile(), and let execute() to invoke it.    
> 
> That's what I'm asking for.
> 
> > I said the reason why I don't choose (1). I prefer (2) because it brings minimal modification. Do you mean you prefer (3)? I just concern there is too big modification.
> 
> (3) is the only sensible path.

Ok, I understand what you mean now. I'll try to make a new patch.

[Geoffrey Garen]

Comment on attachment 152134 [details]
Patch

It looks like this patch will recompile a top-level program and then replace all global functions declared by that program using the function keyword. Is that really how this feature is supposed to work? What happens to non-global functions and functions declared using the var keyword?

[Peter Wang]

(In reply to comment #67)
> (From update of attachment 152134 [details])
> It looks like this patch will recompile a top-level program and then replace all global functions declared by that program using the function keyword. Is that really how this feature is supposed to work? What happens to non-global functions and functions declared using the var keyword?

Sorry for reply your comments so late. I did a test again, my patch can pass the test cases you mentioned. Since the Identifier of old function is replaced, the function body will also be recompiled.

[Geoffrey Garen]

Created attachment 153900 [details]
test case

I believe this test case will fail, given your approach to code editing.

[Peter Wang]

(In reply to comment #69)
> Created an attachment (id=153900) [details]
> test case
> 
> I believe this test case will fail, given your approach to code editing.

I did a test, my patch can pass this test. :)

[Peter Wang]

Created attachment 155436 [details]
Patch

[Build Bot]

Comment on attachment 155436 [details]
Patch

Attachment 155436 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13390639

[Build Bot]

Comment on attachment 155436 [details]
Patch

Attachment 155436 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13387652

[Peter Wang]

Created attachment 155446 [details]
Patch

[Build Bot]

Comment on attachment 155446 [details]
Patch

Attachment 155446 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13392599

[Peter Wang]

Created attachment 155450 [details]
Patch

[Build Bot]

Comment on attachment 155450 [details]
Patch

Attachment 155450 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13385801

[Peter Wang]

Created attachment 155460 [details]
Patch

[Build Bot]

Comment on attachment 155460 [details]
Patch

Attachment 155460 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13396564

[Peter Wang]

Created attachment 155472 [details]
Patch

[Geoffrey Garen]

> > I believe this test case will fail, given your approach to code editing.
> 
> I did a test, my patch can pass this test. :)

I don't think you ran the test right. I don't see any code in this patch that would make it work.

[Peter Wang]

(In reply to comment #81)
> > > I believe this test case will fail, given your approach to code editing.
> > 
> > I did a test, my patch can pass this test. :)
> 
> I don't think you ran the test right. I don't see any code in this patch that would make it work.

Sorry, I did a test again with newest code, my patch doesn't pass this test, and V8 can pass it. :(

However, the reason is not that the embedded functions aren't be recompiled. In JSC, the expression  
var aa = function button1Click() {...};
is compiled as there is a function declaration with name "aa" (refer to JSC::AssignResolveNode::emitBytecode). And we need "execute" these bytecodes to make "aa" to become related with the new function body.

The book "JavaScript: The Definitive Guide" says, for that type of expression, like 
var aa = function button1Click() {...}; 
"aa" should work like a pointer to the function. 
I'm not expert of javascript syntax, maybe it's a defect of JSC implementation?

[Geoffrey Garen]

> I'm not expert of javascript syntax, maybe it's a defect of JSC implementation?

No, you've just misdiagnosed the issue.

[Peter Wang]

(In reply to comment #83)
> > I'm not expert of javascript syntax, maybe it's a defect of JSC implementation?
> 
> No, you've just misdiagnosed the issue.

Sorry, I'm a little confused. You mean:
(1) The way to implementation of "setScriptSource" is wrong, I shouldn't implement it by recompiling?
(2) Or, I don't understand this feature well? I think it gives user a chance to modify some function or expression and see what will happen if these function or expression triggered. Do you think it's right?

[Peter Wang]

(In reply to comment #83)
> > I'm not expert of javascript syntax, maybe it's a defect of JSC implementation?
> 
> No, you've just misdiagnosed the issue.

Agree, I understand it now, thank you very much.

[Pavel Feldman]

It requires a JSC reviewer, changing the component to remove it off the inspector dashboard.

[Timothy Hatcher]

Can someone on the JSC team take a look at the latest patch?

[Oliver Hunt]

Comment on attachment 155472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155472&action=review

I've redone how compilation works since this patch was put up so a lot of the refactoring can probably go away :-/

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1131
> -                    if (i == 0) {
> +                    if (!i) {

Don't change this, while it's a style violation we don't really like changes just to appease the style bot.

[kangax]

Any news on this?

[Peter Wang]

(In reply to comment #89)
> Any news on this?
The patches of mine cannot cover all the cases, so just ignore them. I'm not sure if JSC supports live-editing js code right now. If not, maybe, I should spend sometime to investigate and try again.

[Timothy Hatcher]

JSC does not support live-editing right now. It is a complex problem (especially when closures are involved.)