Bug 105280

(In reply to comment #0) > 'line' sometimes ends up being set to a value other than zero. That should be: _Apparently_ 'line' sometimes ends up being set to a value other than zero -- I have not been able to catch this in gdb.

Interesting. There must be some case when the XHR callback comes through in which the ScriptableDocumentParser thinks it's active. I'll try to reproduce this locally, but it looks like it's only failing on GTK and EFL (see http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fxmlhttprequest%2Forigin-whitelisting-https.html).

It's not just XHR: http/tests/security/canvas-remote-read-remote-image-redirect.html also does this: -CONSOLE MESSAGE: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. -CONSOLE MESSAGE: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. -CONSOLE MESSAGE: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. -CONSOLE MESSAGE: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. +CONSOLE MESSAGE: line 108: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. +CONSOLE MESSAGE: line 108: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. +CONSOLE MESSAGE: line 108: Unable to get image data from canvas because the canvas has been tainted by cross-origin data. +CONSOLE MESSAGE: line 108: Unable to get image data from canvas because the canvas has been tainted by cross-origin data.

This very same issue happens every now and then (flaky) in the GTK Debug bot

> http/tests/security/canvas-remote-read-remote-image-redirect.html also does this: Filed bug 121458 before finding this one.

This happens on the GTK release bot/build at randomly intervals. At least the following tests are affected: http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html http/tests/xmlhttprequest/access-control-preflight-async-not-supported.html http/tests/xmlhttprequest/cross-site-denied-response.html http/tests/xmlhttprequest/origin-whitelisting-https.html http/tests/xmlhttprequest/simple-cross-origin-denied-events.html http/tests/xmlhttprequest/simple-cross-origin-denied-events-post.html

Also on Mac debug bots: http/tests/websocket/tests/hybi/reserved-bits.html http/tests/websocket/tests/hybi/deflate-frame-invalid-parameter.html http/tests/websocket/tests/hybi/handshake-fail-by-more-protocol-header.html

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

Created attachment 244821 [details] proposed fix

Comment on attachment 244821 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=244821&action=review > Source/WebCore/dom/ScriptableDocumentParser.h:51 > + virtual bool consoleMessagesShouldBeAssociatedToParser() const = 0; I don’t think “associated to” is good grammar. Should be “associated with” like the comment. Also, since this is a parser function, I suggest “associated with text position” rather than “associated to parser”. Maybe: virtual bool shouldAssociateConsoleMessagesWithTextPosition() const = 0; > Source/WebCore/html/parser/HTMLDocumentParser.h:64 > + virtual bool consoleMessagesShouldBeAssociatedToParser() const override final; I suggest making this private since there is no need to call it on HTMLDocumentParser. Could always make it public later. > Source/WebCore/page/PageConsoleClient.cpp:91 > + // We definitely cannot associate the message to a location being parsed if we are not even parsing. “with a location” rather than “to a location”. > Source/WebCore/page/PageConsoleClient.cpp:95 > + ScriptableDocumentParser* parser = document->scriptableDocumentParser(); What guarantees this is never null? If it’s guaranteed not to be null, why are we putting it in a pointer rather than a reference? > Source/WebCore/page/PageConsoleClient.cpp:111 > unsigned line = 0; > unsigned column = 0; I suggest putting the initialization to 0 inside the getParserLocationForConsoleMessage function.

> What guarantees this is never null? If it’s guaranteed not to be null, why are we putting it in a pointer rather than a reference? I do not have a complete guarantee, however we check for parsing() before doing this, and perhaps more importantly, all regression tests pass. I didn't want to add a null check just in case.

> I suggest putting the initialization to 0 inside the getParserLocationForConsoleMessage function. I don't think that I agree. The 0/0 is not a parser location, so a "get parser location" function is not the right place to set that. To be more generally useful, the function could return true/false, not an in-band failure signal. But we don't need it to be more generally useful.

Committed <http://trac.webkit.org/r178648>.

(In reply to comment #12) > > I suggest putting the initialization to 0 inside the getParserLocationForConsoleMessage function. > > I don't think that I agree. The 0/0 is not a parser location, so a "get > parser location" function is not the right place to set that. Sure, but a function that conditionally leaves out arguments untouched is not a good design pattern either. I think the function should set the values to something well defined. And setting them to 0/0 would be good for all current clients. As you say, if we wanted to generalize it for more clients later we could do something even fancier, but I don’t think it’s good to have a function with a contract that says “only sometimes set the values of these out arguments” unless there is some compelling advantage to doing so.

(In reply to comment #11) > > What guarantees this is never null? If it’s guaranteed not to be null, why are we putting it in a pointer rather than a reference? > > I do not have a complete guarantee, however we check for parsing() before > doing this, and perhaps more importantly, all regression tests pass. I > didn't want to add a null check just in case. That’s a fine answer to my first question. What’s the answer to my second question?

> I don’t think it’s good to have a function with a contract that says “only sometimes set the values of these out arguments” unless there is some compelling advantage to doing so. I thought that most "get" functions in WebKit did this, leaving the output untouched when failing. And even when there is a boolean result returned, it's still up to the caller to not accidentally use the uninitialized output arguments. See e.g. ApplicationCacheHost::getApplicationCacheFallbackResource() or JSObject::getOwnPropertySlot(). This hasn't been a major source of problems in WebKit historically. I don't see this case as being substantially different. > What’s the answer to my second question? I assume that you meant changing that line to: ScriptableDocumentParser& parser = *document->scriptableDocumentParser(); (or possibly auto&, which in my opinion makes it hard to see what types we are working on). Transforming the result type would increase cognitive load on the reader here. Suddenly, they would have to jump between pointers and references, all just to make a couple member function calls. To me, making a reference on the fly is not a convincing way of saying "this is really non-null, shouldn't raise red flags when reading".

(In reply to comment #16) > I assume that you meant changing that line to: > > ScriptableDocumentParser& parser = *document->scriptableDocumentParser(); > > (or possibly auto&, which in my opinion makes it hard to see what types we > are working on). > > Transforming the result type would increase cognitive load on the reader > here. Suddenly, they would have to jump between pointers and references, all > just to make a couple member function calls. > > To me, making a reference on the fly is not a convincing way of saying "this > is really non-null, shouldn't raise red flags when reading". I think what you might be missing is that we are transitioning to making these functions return references. So the local reference variable today is a “jump” between pointers and references, only until we change the function to return a reference, since it’s a function that is not going to return null. Code processing something that can’t be null should change to references, and in some cases during the transition, the function it’s calling might still be a pointer. I was hoping to recruit you to help in this transition whenever you modify code or write new code. If the thing really isn’t guaranteed to be non-null, it’s a whole different problem, and not part of the transition.

Reopening due to flaky tests.

Wrong bug.