Bug 74115 – Web Inspector: XMLHttpRequest unsafe cross origin access errors for should show stack trace in console.

• Status: UNCONFIRMED
• Component: Web Inspector

[Vsevolod Vlasov]

XMLHttpRequest unsafe cross origin access errors for should show stack trace in console.

[Vsevolod Vlasov]

Extracted from https://bugs.webkit.org/show_bug.cgi?id=73099.

Copying pfeldman's review message:

> Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:41
> +#include "ScriptCallStack.h"

Please mind the alphabetic order

> Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:118
> +    impl()->setCallStack(createScriptCallStack(exec));

I think createScriptCallStack is a bad name for the method, sorry I missed the moment it got landed. You should explicitly state that the stack depth depends on the inspector front-end existence.

> Source/WebCore/bindings/v8/V8Proxy.cpp:166
> +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

I don't see a corresponding change to JSC. Is this a part of a different change? You should probably limit the scope of this change to XHR and keep symmetry here.

> Source/WebCore/xml/XMLHttpRequest.h:128
> +    void setCallStack(PassRefPtr<ScriptCallStack>);

setLastSendCallStack ?

> Source/WebCore/xml/XMLHttpRequest.h:228
> +    RefPtr<ScriptCallStack> m_callStack;

m_lastSendCallStack

[Vsevolod Vlasov]

> > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:41
> > +#include "ScriptCallStack.h"
> 
> Please mind the alphabetic order
Done.

> > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:118
> > +    impl()->setCallStack(createScriptCallStack(exec));
> 
> I think createScriptCallStack is a bad name for the method, sorry I missed the moment it got landed. You should explicitly state that the stack depth depends on the inspector front-end existence.
Renamed in a separate patch.

> > Source/WebCore/bindings/v8/V8Proxy.cpp:166
> > +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> 
> I don't see a corresponding change to JSC. Is this a part of a different change? You should probably limit the scope of this change to XHR and keep symmetry here.
Moved XHR part out to the separate bug.

> > Source/WebCore/xml/XMLHttpRequest.h:128
> > +    void setCallStack(PassRefPtr<ScriptCallStack>);
> 
> setLastSendCallStack ?
> 
> > Source/WebCore/xml/XMLHttpRequest.h:228
> > +    RefPtr<ScriptCallStack> m_callStack;
> 
> m_lastSendCallStack

Renamed to lastCallStack / setLastCallStack.
Please note that this field is not only used for send, it is also set in setRequestHeader

[Vsevolod Vlasov]

Created attachment 118459 [details]
Patch

[Vsevolod Vlasov]

Created attachment 118460 [details]
Patch

[Adam Barth]

Comment on attachment 118460 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.idl:70
> -        void setRequestHeader(in DOMString header, in DOMString value)
> +        [CustomArgumentHandling] void setRequestHeader(in DOMString header, in DOMString value)

Should we have a CallWith=CallStack ?

[Pavel Feldman]

Comment on attachment 118460 [details]
Patch

+1 to Adam's comment, but this can probably be done in a separate change.

[Alexey Proskuryakov]

> PassRefPtr<ScriptArguments>, PassRefPtr<ScriptCallStack> callStack, ExceptionCode& ec

I think that it's quite ugly to pollute DOM code with tons of script related variables. Can this be abstracted away?

Generating proper line numbers for DOM errors is of course something we wanted for a long time, and it's not just XMLHttpRequest. Are we going to have these arguments and CustomArgumentHandling in most DOM functions?

[Vsevolod Vlasov]

(In reply to comment #5)
> (From update of attachment 118460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118460&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.idl:70
> > -        void setRequestHeader(in DOMString header, in DOMString value)
> > +        [CustomArgumentHandling] void setRequestHeader(in DOMString header, in DOMString value)
> 
> Should we have a CallWith=CallStack ?

CallWith adds arguments before DOM arguments. It's probably not nice to add CallStack after DOM arguments, but I think it's even worse to add it before.
I agree that we need to add a new attribute for that though.
I had another look on CustomArgumentHandling implementation and noticed that it returns early when there is no callStack available which is not correct for XMLHttpRequest methods. 

(In reply to comment #7)
> > PassRefPtr<ScriptArguments>, PassRefPtr<ScriptCallStack> callStack, ExceptionCode& ec
> 
> I think that it's quite ugly to pollute DOM code with tons of script related variables. Can this be abstracted away?
> 
> Generating proper line numbers for DOM errors is of course something we wanted for a long time, and it's not just XMLHttpRequest. Are we going to have these arguments and CustomArgumentHandling in most DOM functions?

Essentially there are two types of DOM functions:
1) synchronous calls - in this case call stack can be generated once it is needed. V8 supports that, but for JSC we need ExecState to generate call stack (https://bugs.webkit.org/show_bug.cgi?id=40118).
To workaround that we can either pass ExecState/CallStack to DOM functions as an argument (like in this patch), or store it in the DOM object field and use when needed.

2) asynchronous calls - in this case we need to prepare call stack in advance because we might need it in an asynchronous callback. 

I suggest we add two attributes:

[SaveCallStackBeforeCall] - will generate imp->setLastCallStack(createCallStackForInspector()) before function call. This is not currently needed though, since send() has [Custom] implementation in XMLHttpRequest.

[SaveExecStateBeforeCall] - will call imp->setLastExecState(execState) and imp->setLastExecState(0) before and after call (in JSC only and can be removed once https://bugs.webkit.org/show_bug.cgi?id=40118 is fixed).

Methods setLastExecState and setLastCallStack can be inhertied from CallStackKeeper class.

class CallStackKeeper {
public:
    void setLastCallStack(PassRefPtr<ScriptCallStack>);
    ScriptCallStack* getLastCallStack();
#if USE(JSC)
    void setLastExecState(ExecState*);
    ExecState* getLastExecState();
#endif
private:
    RefPtr<ScriptCallStack> m_lastCallStack;
    ExecState* m_lastExecState;
};

What do you think?

[Alexey Proskuryakov]

I've been thinking that a more complete bindings information class could replace ExceptionCode for functions that need to pass more bindings specific info. I don't have any detailed design for that.

> 2) asynchronous calls - in this case we need to prepare call stack in advance because we might need it in an asynchronous callback. 

How would that affect performance?

[johnjbarton]

Mike West: related to your devMsgs, had r+ a year ago, maybe just needs rebase.

[Vsevolod Vlasov]

Comment on attachment 118460 [details]
Patch

It is not enough to rebase it. There were concerns that this is polluting the code as you can see above. I didn't have time to finish this patch yet. Clearing r+ for now.

[Blaze Burg]

Migrating

[Radar WebKit Bug Importer]

<rdar://problem/16460986>