Web Inspector: Pipe requests through to Inspector in order to add "initiator" links to messages.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Mike West]

Messages associated with requests can generally be improved by linking to the point at which the message was generated. I had a quick conversation with Pavel about this, he suggested to file a bug to continue the discussion.

As an example, the mixed content warning "The page at XXX displayed insecure content from YYY." should link off to the element or code snippet that caused the insecure resource to load (see https://mkw.st/p/mixed/ for an example of the current state).  This will be partially resolved with 97979, which adds a call stack when relevant, but won't help when the resource is loaded by the parser. If no call stack is present, it would make sense to use that space to link to the request's initiator.

I've dug around a bit to see how this might work. The only place I've found that creates a console message with a requestID set is the mixed MIME type error from inside the inspector (NetworkManager.js:191). Is there a mechanism to bind requests and console messages together when creating the message from WebCore?

If not, I think the best I can do is to do a 'resourceForURL' lookup for every URL I see in the message, which seems error-prone.

WDYT?

[Pavel Feldman]

See ConsoleMessage.cpp:148, it does:

jsonObj->setNetworkRequestId(m_requestId)

[Mike West]

(In reply to comment #1)
> See ConsoleMessage.cpp:148, it does:
> 
> jsonObj->setNetworkRequestId(m_requestId)

Got it, thanks! At the moment, the only mechanisms to pass a request identifier into the inspector seems to be ' InspectorConsoleAgent::didFinishXHRLoading', 'InspectorConsoleAgent::didReceiveResponse', and 'InspectorConsoleAgent::didFailLoading'.

I'd like to expose a new variant of 'addMessageToConsole' to allow passing a request identifier in from various other points in WebCore. I'll spin up a strawman patch for discussion soonish.

[Mike West]

Created attachment 170919 [details]
Adding a new 'addMessageToConsole' method.

[Mike West]

Comment on attachment 170919 [details]
Adding a new 'addMessageToConsole' method.

Not for landing, but a strawman of how this might look.

[Mike West]

Created attachment 170920 [details]
How usage might look.

Again, not for landing, but an example of how the new addToConsole method might be useful.

[Pavel Feldman]

Comment on attachment 170919 [details]
Adding a new 'addMessageToConsole' method.

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

> Source/WebCore/page/Console.h:58
> +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL = String(), unsigned lineNumber = 0, PassRefPtr<ScriptCallStack> = 0, unsigned long identifier = 0);

requestIdentifier?

> Source/WebCore/page/Console.h:60
> +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, unsigned long identifier, const String& url);

Why do we need this one? Is this convenience method? It sounds like its arguments fall into a subset of the one you are adjusting above.

[Mike West]

(In reply to comment #6)
> (From update of attachment 170919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170919&action=review
> 
> > Source/WebCore/page/Console.h:58
> > +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL = String(), unsigned lineNumber = 0, PassRefPtr<ScriptCallStack> = 0, unsigned long identifier = 0);
> 
> requestIdentifier?

Yup. That's a better name.

> 
> > Source/WebCore/page/Console.h:60
> > +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, unsigned long identifier, const String& url);
> 
> Why do we need this one? Is this convenience method? It sounds like its arguments fall into a subset of the one you are adjusting above.

Just convenience. It seemed reasonable to have a separate method for resource-related console messages for the same reasons it's nice to have a separate method for url/line-number and stacktrace messages.

[Pavel Feldman]

Comment on attachment 170919 [details]
Adding a new 'addMessageToConsole' method.

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

>>> Source/WebCore/page/Console.h:60
>>> +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, unsigned long identifier, const String& url);
>> 
>> Why do we need this one? Is this convenience method? It sounds like its arguments fall into a subset of the one you are adjusting above.
> 
> Just convenience. It seemed reasonable to have a separate method for resource-related console messages for the same reasons it's nice to have a separate method for url/line-number and stacktrace messages.

You don't need another Instrumentation method then - we only want such convenience for the call sites, not inside the inspector guts.

[Mike West]

(In reply to comment #8)
> (From update of attachment 170919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170919&action=review
> 
> >>> Source/WebCore/page/Console.h:60
> >>> +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, unsigned long identifier, const String& url);
> >> 
> >> Why do we need this one? Is this convenience method? It sounds like its arguments fall into a subset of the one you are adjusting above.
> > 
> > Just convenience. It seemed reasonable to have a separate method for resource-related console messages for the same reasons it's nice to have a separate method for url/line-number and stacktrace messages.
> 
> You don't need another Instrumentation method then - we only want such convenience for the call sites, not inside the inspector guts.

You're right; this would be covered by the new method on Document. I'll spin a new patch.

[Mike West]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 170919 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170919&action=review
> > 
> > >>> Source/WebCore/page/Console.h:60
> > >>> +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, unsigned long identifier, const String& url);
> > >> 
> > >> Why do we need this one? Is this convenience method? It sounds like its arguments fall into a subset of the one you are adjusting above.
> > > 
> > > Just convenience. It seemed reasonable to have a separate method for resource-related console messages for the same reasons it's nice to have a separate method for url/line-number and stacktrace messages.
> > 
> > You don't need another Instrumentation method then - we only want such convenience for the call sites, not inside the inspector guts.
> 
> You're right; this would be covered by the new method on Document. I'll spin a new patch.

Actually, now that I look at the Instrumentation code again, I'm not sure I understand your suggestion.

Currently, we have two InspectorInstrumentation::addMessageToConsole methods: one for line numbered messages, and one for stack traces (well, four, since those two are replicated for workers). This patch adds another for request identifiers.

Are you suggesting that those should be combined into one method that accepts all relevant arguments, and decides which ConsoleMessage to construct based upon the given parameters?

[Mike West]

Created attachment 171204 [details]
Patch

[Mike West]

After a bit of discussion with Pavel (thanks!), I've cleaned up this patch, and updated 'InspectorConsoleAgent::didFinishXHRLoading', 'InspectorConsoleAgent::didReceiveResponse', and 'InspectorConsoleAgent::didFailLoading' to use the new parameter on 'addMessageToConsole' rather than replicating the work themselves. That should test that the existing functionality works as expected.

I'll repurpose 97979 to use this functionality once it lands, which will still more or less look like https://bugs.webkit.org/attachment.cgi?id=170920.

WDYT?

[Pavel Feldman]

Comment on attachment 171204 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.h:86
> +    void addConsoleMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL = String(), unsigned lineNumber = 0, PassRefPtr<ScriptCallStack> = 0, unsigned long requestIdentifier = 0);

Are there any immediate clients for this one?

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:159
> +    String requestId = requestIdentifier ? IdentifiersFactory::requestId(requestIdentifier) : String();

You could push this further and make ConsoleMessage constructor accept unsigned long. You would be able to leave more code below as is then.

[Mike West]

(In reply to comment #13)
> (From update of attachment 171204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171204&action=review
> 
> > Source/WebCore/dom/ScriptExecutionContext.h:86
> > +    void addConsoleMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL = String(), unsigned lineNumber = 0, PassRefPtr<ScriptCallStack> = 0, unsigned long requestIdentifier = 0);
> 
> Are there any immediate clients for this one?

No. I'll need to do some work in the callers to pipe a request identifier in. I'm happy to drop it from the patch and add it in with a client if that's something you'd like to see.

> 
> > Source/WebCore/inspector/InspectorConsoleAgent.cpp:159
> > +    String requestId = requestIdentifier ? IdentifiersFactory::requestId(requestIdentifier) : String();
> 
> You could push this further and make ConsoleMessage constructor accept unsigned long. You would be able to leave more code below as is then.

That makes sense. I'll make this change.

[Mike West]

Created attachment 171210 [details]
Patch

[Mike West]

(In reply to comment #14)

> > > Source/WebCore/inspector/InspectorConsoleAgent.cpp:159
> > > +    String requestId = requestIdentifier ? IdentifiersFactory::requestId(requestIdentifier) : String();
> > 
> > You could push this further and make ConsoleMessage constructor accept unsigned long. You would be able to leave more code below as is then.
> 
> That makes sense. I'll make this change.

I've made half of this change. :)  I pushed the IdentifiersFactory::requestId call down into ConsoleMessage (and adjusted it to return an empty string if provided with 0 as an ID to obviate the 'if' here). I haven't, however, reverted the changes to call addMessageToConsole. It makes more sense to me to centralize the logic about which ConsoleMessage to create in that method, rather than spreading it out over these three. These methods generate a message, but I think they should hand off to the existing method to push it to the console.

WDYT?

[Mike West]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 171204 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171204&action=review
> > 
> > > Source/WebCore/dom/ScriptExecutionContext.h:86
> > > +    void addConsoleMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL = String(), unsigned lineNumber = 0, PassRefPtr<ScriptCallStack> = 0, unsigned long requestIdentifier = 0);
> > 
> > Are there any immediate clients for this one?
> 
> No. I'll need to do some work in the callers to pipe a request identifier in. I'm happy to drop it from the patch and add it in with a client if that's something you'd like to see.

There's now a client of this change in 100735. Would you like me to move the ScriptExecutionContext/Document changes into that patch, or are you ok with landing this patch as-is?

[Pavel Feldman]

Comment on attachment 171210 [details]
Patch

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

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:159
> +    String requestId = requestIdentifier ? IdentifiersFactory::requestId(requestIdentifier) : String();

You don't need it anymore?

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:167
> +    String requestId = requestIdentifier ? IdentifiersFactory::requestId(requestIdentifier) : String();

ditto

[Mike West]

Created attachment 171423 [details]
Patch

[Mike West]

Created attachment 171424 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171424 [details]
Patch for landing

Clearing flags on attachment: 171424

Committed r132918: <http://trac.webkit.org/changeset/132918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.