Bug 100650 – Web Inspector: Autogenerate stack traces and line numbers when possible.

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

[Mike West]

Currently, we ask callsites to generate stack traces or pass in line numbers when generating console messages. Predictably, dozens (hundreds?) of callsites don't.

I'd like to automatically generate stack traces or line numbers when possible. This would basically involve taking the logic that currently lives in 'InspectorResourceAgent::buildInitiatorObject', and moving it somewhere that we could call internally when given a console message that doesn't have information about the context in which it executed.

At the end of the day, I think it makes sense to have three 'addMessage' methods on Document, two explicit, one implicit.

* addMessage(MessageSource source, MessageType type, MessageLevel level, const String& message, PassRefPtr<ScriptCallStack> callStack, unsigned long requestIdentifier = 0)
* addMessage(MessageSource source, MessageType type, MessageLevel level, const String& message, const String& sourceURL, unsigned lineNumber, unsigned long requestIdentifier = 0)
* addMessage(MessageSource source, MessageType type, MessageLevel level, const String& message)

If the last one is called (and the inspector is open and the console is enabled), we'd generate a stack trace. If it's empty, we'd look for a scriptableDocumentParser to get a URL and line number.

I'm very open to suggestion about where that generation should live. I'd agreed with Pavel that putting it directly in Document::addMessage seemed like the wrong place, but now I'm not so sure... that certainly seems like the simplest solution. :)

[Mike West]

Er. I was confused. Replace "three 'addMessage' methods on Document" with "three 'addConsoleMessage' methods on ScriptExecutionContext and its subclasses". And maybe on Console too, now that I'm looking at callsites. :)

[Pavel Feldman]

How about a public static method on ConsoleMessage for non-console clients + its usage in ConsoleMessage's constructor?

[Mike West]

(In reply to comment #2)
> How about a public static method on ConsoleMessage for non-console clients + its usage in ConsoleMessage's constructor?

In order to check the line number, I need a reference to a document to call 'document->scriptableDocumentParser()'. Putting a static method on ConsoleMessage might make sense, but doing the work inside ConsoleMessage's constructor doesn't because we don't have anything that would allow us to get back to the document. I think we need to grab the line number higher in the stack.

How about in Console::addMessage? Console knows what frame it's associated with, which would give us the bits we need. Otherwise I think we'd end up piping the document through InspectorInstrumentation::addMessageToConsole into ConsoleMessage. That seems like more work than it's worth.

WDYT?

[Pavel Feldman]

> WDYT?

Sounds good!

[Mike West]

Created attachment 171670 [details]
Patch

[Mike West]

Hey Pavel, this is a patch so you can see what I'm thinking. Does it more or less line up with what you thought you were agreeing to? :)

Not throwing to the bots yet, as it depends on 100850 landing first.

Thanks!

[Pavel Feldman]

Comment on attachment 171670 [details]
Patch

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

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

So what do these stand for now? Is it parseLine or arbitrary line number identifying the context? If it is the latter, I think I like the former API where message could have arbitrary url + line context and in addition to that, it could have an optional JavaScript stack.

[Build Bot]

Comment on attachment 171670 [details]
Patch

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

[Mike West]

(In reply to comment #7)
> (From update of attachment 171670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171670&action=review
> 
> > Source/WebCore/page/Console.h:59
> > +    void addMessage(MessageSource, MessageType, MessageLevel, const String& message, const String& sourceURL, unsigned lineNumber, unsigned long requestIdentifier = 0);
> 
> So what do these stand for now? Is it parseLine or arbitrary line number identifying the context?

I think it's the latter. Otherwise we would expect a caller to use the callstack version or the completely automatic version.

> If it is the latter, I think I like the former API where message could have arbitrary url + line context and in addition to that, it could have an optional JavaScript stack.

Currently, we're throwing the line number away if we have a stack trace: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/Console.cpp&type=cs&l=152 It makes sense to me to reflect that in the API.

That said, I'm not entirely sure what the current usage looks like. I'm very much open to suggestion about what makes sense.

[WebKit Review Bot]

Comment on attachment 171670 [details]
Patch

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

New failing tests:
http/tests/security/canvas-remote-read-remote-image-redirect.html
http/tests/security/local-image-from-remote.html
http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html
http/tests/security/script-with-failed-cors-check-fails-to-load.html
http/tests/misc/drag-over-iframe-invalid-source-crash.html
http/tests/security/xss-DENIED-xsl-external-entity.xml
http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/misc/image-blocked-src-change.html
http/tests/misc/iframe-invalid-source-crash.html
http/tests/misc/bubble-drag-events.html
http/tests/misc/image-blocked-src-no-change.html
http/tests/misc/unloadable-script.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html
http/tests/security/cross-origin-xsl-BLOCKED.html
http/tests/security/video-poster-cross-origin-crash.html
http/tests/security/img-with-failed-cors-check-fails-to-load.html
http/tests/misc/last-modified-parsing.html
http/tests/security/video-poster-cross-origin-crash2.html
http/tests/inspector/console-xhr-logging.html
http/tests/security/contentSecurityPolicy/default-src-inline-blocked.html
http/tests/security/frame-loading-via-document-write.html
http/tests/security/cross-origin-xsl-redirect-BLOCKED.html
http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html
http/tests/security/xss-DENIED-xsl-document.xml
http/tests/security/contentSecurityPolicy/block-mixed-content-hides-warning.html
http/tests/security/xss-DENIED-xml-external-entity.xhtml

[Mike West]

Created attachment 172324 [details]
WIP #1

[Mike West]

Comment on attachment 172324 [details]
WIP #1

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

Hey Pavel, this is a stab at #1 that we discussed. Dealing with the inspector clients turned out not to be particularly important because they don't currently accept stack traces (we might want to change that).

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:174
> +            ASSERT_NOT_REACHED();

Er. I should have removed this line and the next before uploading. Crashing is a nice way to debug. ;)

[Mike West]

(In reply to comment #12)
> (From update of attachment 172324 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172324&action=review
> 
> Hey Pavel, this is a stab at #1 that we discussed. Dealing with the inspector clients turned out not to be particularly important because they don't currently accept stack traces (we might want to change that).
> 
> > Source/WebCore/inspector/InspectorConsoleAgent.cpp:174
> > +            ASSERT_NOT_REACHED();
> 
> Er. I should have removed this line and the next before uploading. Crashing is a nice way to debug. ;)

Actually, ignore this patch. I'm thinking about it some more, and it might make sense to go another route.

[Mike West]

Created attachment 174985 [details]
Patch

[Mike West]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 172324 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=172324&action=review
> > 
> > Hey Pavel, this is a stab at #1 that we discussed. Dealing with the inspector clients turned out not to be particularly important because they don't currently accept stack traces (we might want to change that).
> > 
> > > Source/WebCore/inspector/InspectorConsoleAgent.cpp:174
> > > +            ASSERT_NOT_REACHED();
> > 
> > Er. I should have removed this line and the next before uploading. Crashing is a nice way to debug. ;)
> 
> Actually, ignore this patch. I'm thinking about it some more, and it might make sense to go another route.

Ok, please don't ignore the new patch. :)

I need to regenerate test results for Chromium (since something will likely break there), but before I go through the trouble of migrating things over to that linux machine over there, I'd like your take on the approach.

The next step will be to get rid of the externally-visible CallStack endpoints, by replacing them with an optional ScriptState*. That will allow me to get rid of the callstack generation in ContentSecurityPolicy, but it's distinct enough from this patch that I'd like to do it separately if that's alright with you.

WDYT?

[WebKit Review Bot]

Comment on attachment 174985 [details]
Patch

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

New failing tests:
http/tests/eventsource/eventsource-bad-mime-type.html
accessibility/presentational-elements-with-focus.html
http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html
canvas/philip/tests/2d.fillStyle.parse.rgba-solid-3.html
http/tests/canvas/philip/tests/security.pattern.cross.html
http/tests/appcache/deferred-events-delete-while-raising.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
http/tests/canvas/philip/tests/security.pattern.canvas.timing.html
http/tests/canvas/philip/tests/security.pattern.image.strokeStyle.html
http/tests/canvas/philip/tests/security.reset.html
canvas/philip/tests/2d.gradient.object.current.html
http/tests/canvas/philip/tests/security.drawImage.image.html
accessibility/aria-toggle-button-with-title.html
http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html
canvas/philip/tests/2d.drawImage.incomplete.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.fillStyle.parse.hsl-6.html
http/tests/canvas/philip/tests/security.pattern.image.fillStyle.html
http/tests/appcache/deferred-events.html
http/tests/canvas/philip/tests/security.drawImage.canvas.html
http/tests/appcache/online-fallback-layering.html
canvas/philip/tests/2d.fillStyle.parse.rgba-solid-4.html
accessibility/removed-continuation-element-causes-crash.html
http/tests/appcache/video.html
http/tests/appcache/foreign-fallback.html
accessibility/aria-fallback-roles.html
canvas/philip/tests/2d.pattern.image.incomplete.html
http/tests/canvas/philip/tests/security.pattern.create.html
http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html
http/tests/eventsource/eventsource-content-type-charset.html

[Mike West]

Created attachment 175171 [details]
Patch

[Mike West]

(In reply to comment #16)
> (From update of attachment 174985 [details])
> Attachment 174985 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14894247
> 
> New failing tests:
> http/tests/eventsource/eventsource-bad-mime-type.html
> accessibility/presentational-elements-with-focus.html
> http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html
> canvas/philip/tests/2d.fillStyle.parse.rgba-solid-3.html
> http/tests/canvas/philip/tests/security.pattern.cross.html
> http/tests/appcache/deferred-events-delete-while-raising.html
> http/tests/appcache/deferred-events-delete-while-raising-timer.html
> http/tests/canvas/philip/tests/security.pattern.canvas.timing.html
> http/tests/canvas/philip/tests/security.pattern.image.strokeStyle.html
> http/tests/canvas/philip/tests/security.reset.html
> canvas/philip/tests/2d.gradient.object.current.html
> http/tests/canvas/philip/tests/security.drawImage.image.html
> accessibility/aria-toggle-button-with-title.html
> http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html
> canvas/philip/tests/2d.drawImage.incomplete.html
> http/tests/cache/cancel-during-failure-crash.html
> canvas/philip/tests/2d.fillStyle.parse.hsl-6.html
> http/tests/canvas/philip/tests/security.pattern.image.fillStyle.html
> http/tests/appcache/deferred-events.html
> http/tests/canvas/philip/tests/security.drawImage.canvas.html
> http/tests/appcache/online-fallback-layering.html
> canvas/philip/tests/2d.fillStyle.parse.rgba-solid-4.html
> accessibility/removed-continuation-element-causes-crash.html
> http/tests/appcache/video.html
> http/tests/appcache/foreign-fallback.html
> accessibility/aria-fallback-roles.html
> canvas/philip/tests/2d.pattern.image.incomplete.html
> http/tests/canvas/philip/tests/security.pattern.create.html
> http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html
> http/tests/eventsource/eventsource-content-type-charset.html

A new patch, now with less crashing on Chromium! :)

[Mike West]

Friendly ping. I'd like to decide how this bit is going to look before I get too far along with the next bit. :)

[Pavel Feldman]

Comment on attachment 175171 [details]
Patch

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

> Source/WebCore/inspector/ConsoleMessage.h:55
> +    ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, const String& u, unsigned li, unsigned long requestIdentifier = 0, Document* = 0);

Why do we still need u and li parameters here?

[Mike West]

Comment on attachment 175171 [details]
Patch

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

>> Source/WebCore/inspector/ConsoleMessage.h:55
>> +    ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, const String& u, unsigned li, unsigned long requestIdentifier = 0, Document* = 0);
> 
> Why do we still need u and li parameters here?

I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance.

[Pavel Feldman]

> I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance.

I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one...

[Mike West]

(In reply to comment #22)
> I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one...

I'll take a look. My intention was to keep the patches small. Dropping URLs and line numbers will touch a few additional files. :)

[Mike West]

Created attachment 176247 [details]
Patch

[Mike West]

(In reply to comment #22)
> > I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance.
> 
> I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one...

Ok. The new patch removes line numbers for most callers of ScriptExecutionContext::addConsoleMessage, and creates a clean pipe down to ConsoleMessage by adding a new variant of the intermediate methods that doesn't accept a line number/url or stack trace. The goal will be to get rid of the other paths.

I'm putting together a doc that shows the remaining call sites that need work; we should talk about them at some point. :)

[Mike West]

Created attachment 176252 [details]
Patch

[Mike West]

(In reply to comment #25)
> (In reply to comment #22)
> > > I want to get rid of them, but I'm not sure I can yet. The intent was to keep the existing behavior if a URL and line number were passed in, then to walk through all the callers in a separate patch and remove them once I'm sure I can. I'm not sure that will be possible for workers, for instance.
> > 
> > I'd consider doing it right away, otherwise the logic behind the line number calculation would require a comprehensive doc. If this can't be done for workers, workers should have separate constructor. But we should not require all: url, line and document in the same one...
> 
> Ok. The new patch removes line numbers for most callers of ScriptExecutionContext::addConsoleMessage, and creates a clean pipe down to ConsoleMessage by adding a new variant of the intermediate methods that doesn't accept a line number/url or stack trace. The goal will be to get rid of the other paths.
> 
> I'm putting together a doc that shows the remaining call sites that need work; we should talk about them at some point. :)

Uploaded too quickly. The latest patch strips line numbers from a few more call sites.

[Adam Barth]

Comment on attachment 176252 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-197
> -        m_document->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, reason, m_handshake->clientOrigin());

Is m_handshake->clientOrigin() always the same as m_document->securityOrigin()?

> Source/WebCore/inspector/ConsoleMessage.cpp:57
> +    : m_source(s)
> +    , m_type(t)
> +    , m_level(l)
> +    , m_message(m)

Please use complete words in variable names.  For example, s -> source; t -> type

> Source/WebCore/inspector/ConsoleMessage.cpp:58
> +    , m_url()

No need to initialize this variable explicitly.  The compiler will do it for you.

> Source/WebCore/inspector/ConsoleMessage.h:58
> +    ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, unsigned long requestIdentifier = 0, Document* = 0);
>      ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, const String& u, unsigned li, unsigned long requestIdentifier = 0);
> -    ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, PassRefPtr<ScriptArguments>, PassRefPtr<ScriptCallStack>, unsigned long requestIdentifier = 0);
> +    ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, PassRefPtr<ScriptCallStack>, unsigned long requestIdentifier = 0);
> +    ConsoleMessage(MessageSource, MessageType, MessageLevel, const String& m, PassRefPtr<ScriptArguments>, ScriptState*, unsigned long requestIdentifier = 0);

m -> message

[Adam Barth]

This looks like an improvement to me, but I'll let someone from inspector-land do the actual review.

[Mike West]

(In reply to comment #28)
> > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-197
> > -        m_document->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, reason, m_handshake->clientOrigin());
> 
> Is m_handshake->clientOrigin() always the same as m_document->securityOrigin()?

I'm not sure. But I'm also not sure it should be passed in as the console message's URL. I suspect it would be better to have the failing URL as part of the console message.

Who would be a good person to talk to about these errors in particular?

> > Source/WebCore/inspector/ConsoleMessage.cpp:57
> > +    : m_source(s)
> > +    , m_type(t)
> > +    , m_level(l)
> > +    , m_message(m)
> 
> Please use complete words in variable names.  For example, s -> source; t -> type

I was thinking about that while copy/pasting... :)

I'll do a quick style patch in a separate bug and rebase this one on top of it.

[Adam Barth]

> Who would be a good person to talk to about these errors in particular?

I would use svn blame to see who added that line of code and ask them.

[Mike West]

(In reply to comment #31)
> > Who would be a good person to talk to about these errors in particular?
> 
> I would use svn blame to see who added that line of code and ask them.

Ha! I'll do that. :)

[Mike West]

(In reply to comment #28)
> (From update of attachment 176252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176252&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-197
> > -        m_document->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, reason, m_handshake->clientOrigin());
> 
> Is m_handshake->clientOrigin() always the same as m_document->securityOrigin()?

The console message is at least two years old. I stopped digging through blame logs after that. :)

Instead, I dug through the code a bit: it looks like WebSocketHandshake::clientOrigin returns the origin of the ScriptExecutionContext in which the WebSocket was created. If the failure is due to a JavaScript error, the resulting console message will have a stack trace, and will set the URL appropriately. If not, I don't think we're actually adding any value to the console message by noting that it happened in the context of the currently active document: leaving the URL empty doesn't really hurt.

I'll ping some random websockets folks to see if anyone has a strong opinion about it.

[Mike West]

(In reply to comment #33)
> I'll ping some random websockets folks to see if anyone has a strong opinion about it.

After looking a bit more, I think we should just change the message here in a separate patch. I've thrown something up at https://bugs.webkit.org/show_bug.cgi?id=103448 that should obviate this portion of the patch we're discussing here, and poked relevant folks for a review.

Thanks for asking the question, Adam. :)

[Mike West]

Created attachment 176424 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176424 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 176424 [details]
Patch

Attachment 176424 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15018370

[Mike West]

Created attachment 176454 [details]
Patch

[Mike West]

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

Moar test expectation updates. Lots of line number dropping out of the expectations, as they're now only generated if the inspector is open. Not sure if I like that.

[WebKit Review Bot]

Comment on attachment 176454 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 176454 [details]
Patch

Attachment 176454 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15029345

[Mike West]

(In reply to comment #41)
> (From update of attachment 176454 [details])
> Attachment 176454 [details] did not pass cr-android-ews (chromium-android):
> Output: http://queues.webkit.org/results/15029345

I don't understand why this doesn't build on the bots. The build error is
    
    Source/WebCore/Modules/websockets/WebSocketChannel.cpp:197:126: error: no matching function for call to 'WebCore::Document::addConsoleMessage(WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, const WTF::String&, WTF::String)'

but that line was killed this morning in http://trac.webkit.org/changeset/135981  I guess the bots haven't caught up yet?  I'll upload another patch in a few hours. :(

[Mike West]

Created attachment 176762 [details]
Rebased.

[Yury Semikhatsky]

Comment on attachment 176762 [details]
Rebased.

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

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:169
> +        document = m_instrumentingAgents->inspectorAgent()->inspectedDocument();

m_instrumentingAgents are supposed to be read only in InspectorInstrumentation.* If you need to access inspectorAgent you can pass it in the constructor of InspectorConsoleAgent. But in this case you seem to need access to the document where the message is being logged, you should pass it directly as it main differ from the document of main frame that inspectedDocument() returns.

> LayoutTests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction-expected.txt:3
> +FAIL gFocusedDocument.activeElement.getAttribute("id") should be start. Was end.

Hmm, the test is failing now, we should avoid such rebaseline.

[Yury Semikhatsky]

(In reply to comment #39)
> (In reply to comment #38)
> > Created an attachment (id=176454) [details] [details]
> > Patch
> 
> Moar test expectation updates. Lots of line number dropping out of the expectations, as they're now only generated if the inspector is open. Not sure if I like that.

Not providing line number where did so is a regression. We would like to see correct line numbers even if inspector wasn't open when the message was logged.

[Mike West]

(In reply to comment #44)
> (From update of attachment 176762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176762&action=review
> 
> > Source/WebCore/inspector/InspectorConsoleAgent.cpp:169
> > +        document = m_instrumentingAgents->inspectorAgent()->inspectedDocument();
> 
> m_instrumentingAgents are supposed to be read only in InspectorInstrumentation.* If you need to access inspectorAgent you can pass it in the constructor of InspectorConsoleAgent. But in this case you seem to need access to the document where the message is being logged, you should pass it directly as it main differ from the document of main frame that inspectedDocument() returns.

Pass it all the way through from Document::addMessage? Hrm. Too bad. This solution seemed clean, but if the inspectedDocument isn't what I thought it was, then I'll need to pipe it through.

> > LayoutTests/fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction-expected.txt:3
> > +FAIL gFocusedDocument.activeElement.getAttribute("id") should be start. Was end.
> 
> Hmm, the test is failing now, we should avoid such rebaseline.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fspatial-navigation :(

Good eye. I'll just let it keep failing.

[Mike West]

Created attachment 176857 [details]
Patch

[Mike West]

Comment on attachment 176857 [details]
Patch

Not fot landing; I need to rebase lots of tests and clean up the changelog.

Still, I'd like to know whether this is the right direction before I do so. Would you mind taking another look? I've stripped out all the layout test changes so the actual code is easier to parse.

[Yury Semikhatsky]

Comment on attachment 176857 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.cpp:326
> +    addMessage(source, type, level, message, requestIdentifier);

I'd rather make this new addConsoleMessage pure virtual method and removed the new addMessage.

> Source/WebCore/page/Console.cpp:145
> +            url = document->url().string();

Only line number depends on the current parser state, you can unconditionally retrieve url from the document if one is provided.

> Source/WebCore/page/Console.cpp:170
> +    else if (!url.isNull() && lineNumber)

Why do you need this branching?

> Source/WebCore/workers/WorkerContext.cpp:289
> +    addMessageToWorkerConsole(source, type, level, message, String(), 0, 0, requestIdentifier);

In case of workers we should probably try to collect stack trace/get url+line here before posting addConsoleMessage to the main thread.

[WebKit Review Bot]

Comment on attachment 176857 [details]
Patch

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

New failing tests:
http/tests/misc/iframe-invalid-source-crash.html
http/tests/security/xssAuditor/embed-tag-code-attribute-2.html
http/tests/security/xssAuditor/embed-tag-null-char.html
http/tests/security/xssAuditor/embed-tag-javascript-url.html
http/tests/security/mixedContent/insecure-css-in-iframe.html
http/tests/misc/drag-over-iframe-invalid-source-crash.html
http/tests/security/xssAuditor/cookie-injection.html
http/tests/security/xssAuditor/embed-tag-control-char.html
http/tests/security/xssAuditor/full-block-javascript-link.html
http/tests/security/cross-origin-xsl-BLOCKED.html
http/tests/security/xssAuditor/full-block-iframe-no-inherit.php
http/tests/security/xssAuditor/full-block-get-from-iframe.html
http/tests/security/xssAuditor/base-href-scheme-relative.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/mixedContent/insecure-css-in-main-frame.html
http/tests/security/xssAuditor/base-href-null-char.html
http/tests/misc/bubble-drag-events.html
http/tests/security/mixedContent/insecure-image-in-main-frame.html
http/tests/misc/image-blocked-src-no-change.html
http/tests/security/xssAuditor/embed-tag.html
http/tests/security/xssAuditor/form-action.html
http/tests/misc/image-blocked-src-change.html
http/tests/security/xssAuditor/base-href.html
http/tests/security/xssAuditor/full-block-iframe-javascript-url.html
http/tests/security/xssAuditor/full-block-link-onclick.html
http/tests/security/frame-loading-via-document-write.html
http/tests/inspector/console-xhr-logging.html
http/tests/security/xssAuditor/embed-tag-code-attribute.html
http/tests/security/xssAuditor/base-href-control-char.html
http/tests/security/xss-DENIED-xml-external-entity.xhtml

[WebKit Review Bot]

Comment on attachment 176857 [details]
Patch

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

New failing tests:
http/tests/misc/iframe-invalid-source-crash.html
http/tests/security/xssAuditor/embed-tag-code-attribute-2.html
http/tests/security/xssAuditor/embed-tag-null-char.html
http/tests/security/xssAuditor/embed-tag-javascript-url.html
http/tests/security/mixedContent/insecure-css-in-iframe.html
http/tests/misc/drag-over-iframe-invalid-source-crash.html
http/tests/security/xssAuditor/cookie-injection.html
http/tests/security/xssAuditor/embed-tag-control-char.html
http/tests/security/xssAuditor/full-block-javascript-link.html
http/tests/security/cross-origin-xsl-BLOCKED.html
http/tests/security/xssAuditor/full-block-iframe-no-inherit.php
http/tests/security/xssAuditor/full-block-get-from-iframe.html
http/tests/security/xssAuditor/base-href-scheme-relative.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/mixedContent/insecure-css-in-main-frame.html
http/tests/security/xssAuditor/base-href-null-char.html
http/tests/misc/bubble-drag-events.html
http/tests/security/mixedContent/insecure-image-in-main-frame.html
http/tests/misc/image-blocked-src-no-change.html
http/tests/security/xssAuditor/embed-tag.html
http/tests/security/xssAuditor/form-action.html
http/tests/misc/image-blocked-src-change.html
http/tests/security/xssAuditor/base-href.html
http/tests/security/xssAuditor/full-block-iframe-javascript-url.html
http/tests/security/xssAuditor/full-block-link-onclick.html
http/tests/security/frame-loading-via-document-write.html
http/tests/inspector/console-xhr-logging.html
http/tests/security/xssAuditor/embed-tag-code-attribute.html
http/tests/security/xssAuditor/base-href-control-char.html
http/tests/security/xss-DENIED-xml-external-entity.xhtml

[Mike West]

Created attachment 176963 [details]
Patch

[Mike West]

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

Patch is getting big. :)

I think this addresses your concerns, Yury. I've rebaselined the tests I can run locally, but I expect more will break. You'll still see a lot of line number disappearing from the SVG tests. I checked those manually; they shouldn't have been generated in the first place, as they're all coming from JavaScript. They're just pointing to "</script>", where the parser is sitting while JS executes.

Would you mind taking another look?

Thanks!

[Build Bot]

Comment on attachment 176963 [details]
Patch

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

New failing tests:
svg/dom/SVGScriptElement/script-onerror-bubbling.svg

[Yury Semikhatsky]

Comment on attachment 176963 [details]
Patch

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

> Source/WebCore/page/Console.cpp:140
> +    String url = document ? document->url().string() : String();

String url;
if (document)
  url = document->url().string();

> LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters-expected.txt:2
> +CONSOLE MESSAGE: Error while parsing the 'sandbox' attribute: 'allow-scriptsallow-forms' is an invalid sandbox flag.

We are still losing console line numbers in many places where it may be useful. Can you avoid such regressions?

[Mike West]

Comment on attachment 176963 [details]
Patch

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

>> LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters-expected.txt:2
>> +CONSOLE MESSAGE: Error while parsing the 'sandbox' attribute: 'allow-scriptsallow-forms' is an invalid sandbox flag.
> 
> We are still losing console line numbers in many places where it may be useful. Can you avoid such regressions?

Here, line 41 is the end of the file: http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters.html We shouldn't have generated it in the first place, as this is all coming from script.

I've spot-checked several locations and come to the same conclusions each time. Looking at the call sites I've modified, it's clear why: most weren't properly testing to see if the line number ScriptableDocumentParser gave out was meaningful (e.g. we're parsing, and we're not in script).

[Mike West]

(In reply to comment #56)
> (From update of attachment 176963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176963&action=review
> 
> >> LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters-expected.txt:2
> >> +CONSOLE MESSAGE: Error while parsing the 'sandbox' attribute: 'allow-scriptsallow-forms' is an invalid sandbox flag.
> > 
> > We are still losing console line numbers in many places where it may be useful. Can you avoid such regressions?
> 
> Here, line 41 is the end of the file: http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/sandboxed-iframe-parsing-space-characters.html We shouldn't have generated it in the first place, as this is all coming from script.
> 
> I've spot-checked several locations and come to the same conclusions each time. Looking at the call sites I've modified, it's clear why: most weren't properly testing to see if the line number ScriptableDocumentParser gave out was meaningful (e.g. we're parsing, and we're not in script).

That's not at all to say I'm totally 100% sure I'm right. If you see other suspicious locations, I'm really happy to check them out. :)

[Mike West]

Created attachment 176994 [details]
Patch

[Mike West]

*** Bug 97979 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 176994 [details]
Patch

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

New failing tests:
http/tests/security/cross-origin-xsl-BLOCKED.html
http/tests/security/xssAuditor/javascript-link-HTML-entities-null-char.html

[Mike West]

Created attachment 177215 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177215 [details]
Patch for landing

Clearing flags on attachment: 177215

Committed r136377: <http://trac.webkit.org/changeset/136377>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike West]

I'll follow up on this with rebaselines for other ports as the bots catch up.

Thanks for the review, Yury!

[Jussi Kukkonen (jku)]

(In reply to comment #64)
> I'll follow up on this with rebaselines for other ports as the bots catch up.

The EFL bots are not green to start with so please ask if something seems fishy. Don't mind the image flakes, we have a snapshot-related problem at the moment, but these seem to have appeared after this patch (http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2):

Unexpected flakiness: text-only failures
  http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html
  http/tests/xmlhttprequest/cross-site-denied-response.html
  http/tests/xmlhttprequest/origin-whitelisting-https.html

Regressions: Unexpected text-only failures
  fast/media/mq-resolution-dpi-dpcm-warning.html
  fast/media/mq-resolution.html
  fast/media/w3c/test_media_queries.html

Regressions: Unexpected crashes (8)
  fast/workers/shared-worker-exception.html [ Crash ]
  fast/workers/shared-worker-script-error.html [ Crash ]
  fast/workers/storage/open-database-set-empty-version-sync.html [ Crash ]
  http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html [ Crash ]
  http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html [ Crash ]
  http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html [ Crash ]
  http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html [ Crash ]
  http/tests/xmlhttprequest/workers/shared-worker-access-control-basic-get-fail-non-simple.html [ Crash ]

[Mike West]

(In reply to comment #65)
> (In reply to comment #64)
> > I'll follow up on this with rebaselines for other ports as the bots catch up.
> 
> The EFL bots are not green to start with so please ask if something seems fishy. Don't mind the image flakes, we have a snapshot-related problem at the moment, but these seem to have appeared after this patch (http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2):
> 
> Unexpected flakiness: text-only failures
>   http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html
>   http/tests/xmlhttprequest/cross-site-denied-response.html
>   http/tests/xmlhttprequest/origin-whitelisting-https.html
> 
> Regressions: Unexpected text-only failures
>   fast/media/mq-resolution-dpi-dpcm-warning.html
>   fast/media/mq-resolution.html
>   fast/media/w3c/test_media_queries.html
> 
> Regressions: Unexpected crashes (8)
>   fast/workers/shared-worker-exception.html [ Crash ]
>   fast/workers/shared-worker-script-error.html [ Crash ]
>   fast/workers/storage/open-database-set-empty-version-sync.html [ Crash ]
>   http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html [ Crash ]
>   http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html [ Crash ]
>   http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html [ Crash ]
>   http/tests/xmlhttprequest/workers/access-control-basic-get-fail-non-simple.html [ Crash ]
>   http/tests/xmlhttprequest/workers/shared-worker-access-control-basic-get-fail-non-simple.html [ Crash ]

*sigh* Yes. It's crashing on the Apple port as well in an ASSERT. I should have tested on a debug build :(

I'll revert it and figure things out. Bleh.

Thanks!

[WebKit Review Bot]

Re-opened since this is blocked by bug 103881

[Mike West]

Created attachment 177254 [details]
Now with less crashing.

[Mike West]

Hey Yury, mind having another look at this? The only change in the most recent patch is a change to the way that we generate stacks for workers.

Previously, I called out to createScriptCallStack directly, which ends up (in JSC) in an ASSERT that we're on the main thread. That doesn't work so well. This patch generates a ScriptState* from the WorkerContext, and pipes that through the instrumentation bits into ConsoleMessage, which generate the stack directly from the script state when available.

I've tested this in debug builds locally; it works. I'd appreciate your feedback as to whether it's also correct. :)

[Yury Semikhatsky]

Comment on attachment 177254 [details]
Now with less crashing.

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

> Source/WebCore/inspector/ConsoleMessage.cpp:118
> +    m_callStack = state ? createScriptCallStackForConsole(state) : createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true) still can fail in case of workers if someone forgets to pass non-0 state?

> Source/WebCore/workers/WorkerContext.cpp:309
> +        InspectorInstrumentation::addMessageToConsole(this, source, type, level, message, sourceURL, lineNumber, scriptStateFromWorkerContext(this), requestIdentifier);

Retrieving script state from worker context will not give you access to the top frame and so you will always have empty stack trace in such cases. You should use ScriptState* that is available where the message is logged just like you do in case of document. There is no  'console' binding in worker contexts so far but we should either provide correct implementation or postpone that until when we add 'console' binding to workers.

[Mike West]

(In reply to comment #70)
> createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true) still can fail in case of workers if someone forgets to pass non-0 state?

Yes. At the moment there's only one call site (everything goes through WorkerContext::addMessage, so I'm not too worried about it.

What would you suggest?

> > Source/WebCore/workers/WorkerContext.cpp:309
> > +        InspectorInstrumentation::addMessageToConsole(this, source, type, level, message, sourceURL, lineNumber, scriptStateFromWorkerContext(this), requestIdentifier);
> 
> Retrieving script state from worker context will not give you access to the top frame and so you will always have empty stack trace in such cases. You should use ScriptState* that is available where the message is logged just like you do in case of document.

I'm not sure I understand this point. The only place I'm grabbing ScriptState objects at the moment are in the bindings. I'm not actually sure how to get a ScriptState object from inside Document or Console where I'd need it. ScriptState::forContext only exists in V8 bindings, and ScriptState::scriptStateFromPage requires a DOMWrapperWorld that I also don't know how to get access to (and has a comment in the V8 implementation that says more or less that it should only be used for tests).

This was the best I came up with today. I'd really appreciate pointers in the right direction, since it sounds like the solution I found isn't great. :)

> There is no  'console' binding in worker contexts so far but we should either provide correct implementation or postpone that until when we add 'console' binding to workers.

Given that it doesn't make the situation for workers any worse than it already is, I'd suggest splitting any further work on the API into a separate patch (though, of course I'd say that... ;)).

Thanks!

[Mike West]

(In reply to comment #71)
> I'm not sure I understand this point. The only place I'm grabbing ScriptState objects at the moment are in the bindings.

*the only place I'm grabbing ScriptState before this last patch. This patch, of course, grabs ScriptState inside WorkerContext::addMessageToConsole. ;)

[Yury Semikhatsky]

(In reply to comment #71)
> (In reply to comment #70)
> > createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true) still can fail in case of workers if someone forgets to pass non-0 state?
> 
> Yes. At the moment there's only one call site (everything goes through WorkerContext::addMessage, so I'm not too worried about it.
> 
> What would you suggest?
> 
We should not crash here. One way to avoid this would be to check if we're on the main thread in createScriptCallStack in case of JSC and bail out if not.

> > > Source/WebCore/workers/WorkerContext.cpp:309
> > > +        InspectorInstrumentation::addMessageToConsole(this, source, type, level, message, sourceURL, lineNumber, scriptStateFromWorkerContext(this), requestIdentifier);
> > 
> > Retrieving script state from worker context will not give you access to the top frame and so you will always have empty stack trace in such cases. You should use ScriptState* that is available where the message is logged just like you do in case of document.
> 
> I'm not sure I understand this point. The only place I'm grabbing ScriptState objects at the moment are in the bindings. I'm not actually sure how to get a ScriptState object from inside Document or Console where I'd need it.

You need to pass it there explicitly if you have one. My point was that it doen't make much sense to get ScriptState for worker unless you are in the bindings as it won't let you generate a call stack anyway.

Is it possible to change Source/WebCore/inspector/ConsoleMessage.cpp:118 so that it only generated stack trace if script state is not null?


> ScriptState::forContext only exists in V8 bindings, and ScriptState::scriptStateFromPage requires a DOMWrapperWorld that I also don't know how to get access to (and has a comment in the V8 implementation that says more or less that it should only be used for tests).
> 
scriptStateFromPage is not supposed to work for workers as it name suggests.


> This was the best I came up with today. I'd really appreciate pointers in the right direction, since it sounds like the solution I found isn't great. :)
> 
> > There is no  'console' binding in worker contexts so far but we should either provide correct implementation or postpone that until when we add 'console' binding to workers.
> 
> Given that it doesn't make the situation for workers any worse than it already is, I'd suggest splitting any further work on the API into a separate patch (though, of course I'd say that... ;)).
> 
> Thanks!

I agree.

[Mike West]

Created attachment 177556 [details]
Patch

[Mike West]

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

Thanks, Yury. The current patch is the result of our discussion this evening: it adds an 'isWorkerAgent' method to InspectorConsoleAgent, and uses it to determine whether a stack should be generated. This avoids the crashes we saw earlier, at least on my machine. :)

WDYT?

[Mike West]

Created attachment 177579 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 177579 [details]
Patch

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

> Source/WebCore/inspector/ConsoleMessage.cpp:116
> +    if (!canGenerateCallStack)

We should check this only if state is null to avoid crash in createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); if state is presented then it is ok to collect it call stack even in case of workers or am I missing something?

[Mike West]

Created attachment 177671 [details]
Patch

[Mike West]

(In reply to comment #77)
> (From update of attachment 177579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177579&action=review
> 
> > Source/WebCore/inspector/ConsoleMessage.cpp:116
> > +    if (!canGenerateCallStack)
> 
> We should check this only if state is null to avoid crash in createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); if state is presented then it is ok to collect it call stack even in case of workers or am I missing something?

Sure, I can do that. It's in the latest patch.

WDYT?

[Yury Semikhatsky]

Comment on attachment 177671 [details]
Patch

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

> Source/WebCore/inspector/ConsoleMessage.cpp:122
> +        m_callStack = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

It is enough to capture only top frame if you need just url and line number.

> Source/WebCore/inspector/ConsoleMessage.cpp:133
> +    m_callStack.clear();

Why do you clear call stack here?

[Mike West]

For clarity, this is what we discussed via chat:

(In reply to comment #80)
> (From update of attachment 177671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177671&action=review
> 
> > Source/WebCore/inspector/ConsoleMessage.cpp:122
> > +        m_callStack = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> 
> It is enough to capture only top frame if you need just url and line number.

I think we want the whole stack to display to the developer. Grabbing only the top frame wouldn't be detailed enough.

> > Source/WebCore/inspector/ConsoleMessage.cpp:133
> > +    m_callStack.clear();
> 
> Why do you clear call stack here?

Given the 'return' in the if block above, I only clear the stack if it's either already null or if it's non-null but empty. If we couldn't generate a usable stack, I wanted to ensure that we didn't try to display it to the user.

[Mike West]

Comment on attachment 177671 [details]
Patch

Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :)

I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :)

[Mike West]

(In reply to comment #82)
> (From update of attachment 177671 [details])
> Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :)
> 
> I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :)

EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>.

[Mike West]

(In reply to comment #83)
> (In reply to comment #82)
> > (From update of attachment 177671 [details] [details])
> > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :)
> > 
> > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :)
> 
> EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>.

QT in r136670: <http://trac.webkit.org/changeset/136670>.

[Mike West]

(In reply to comment #84)
> (In reply to comment #83)
> > (In reply to comment #82)
> > > (From update of attachment 177671 [details] [details] [details])
> > > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :)
> > > 
> > > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :)
> > 
> > EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>.
> 
> QT in r136670: <http://trac.webkit.org/changeset/136670>.

GTK in r136672: <http://trac.webkit.org/changeset/136672>.

[Mike West]

(In reply to comment #85)
> (In reply to comment #84)
> > (In reply to comment #83)
> > > (In reply to comment #82)
> > > > (From update of attachment 177671 [details] [details] [details] [details])
> > > > Since the CQ is down, I landed this manually as r136657: <http://trac.webkit.org/changeset/136657>. Since webkit-patch didn't clear flags, I'm doing it myself. :)
> > > > 
> > > > I'll watch the bots today to a) make sure it doesn't crash anymore, and b) rebaseline tests from ports I don't have access to. Keeping my fingers crossed! :)
> > > 
> > > EFL rebaselined in r136666: <http://trac.webkit.org/changeset/136666>.
> > 
> > QT in r136670: <http://trac.webkit.org/changeset/136670>.
> 
> GTK in r136672: <http://trac.webkit.org/changeset/136672>.

And minor Apple/Chromium cleanup in r136673: <http://trac.webkit.org/changeset/136673>. Should be the last of them. :) *whew*

[Jussi Kukkonen (jku)]

This seems to have made a bunch of xhr tests flaky for at least EFL:

-CONSOLE MESSAGE: XMLHttpRequest cannot load http://localhost:8000/xmlhttprequest/resources/get.txt. Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.
+CONSOLE MESSAGE: line 58: XMLHttpRequest cannot load http://localhost:8000/xmlhttprequest/resources/get.txt. Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.

This happens ~10% of the time. http/tests/xmlhttprequest/origin-whitelisting-https.html is an example.

[Jussi Kukkonen (jku)]

(In reply to comment #87)
> This seems to have made a bunch of xhr tests flaky for at least EFL:

Bug 105280 filed.