--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/security/contentSecurityPolicy/register-bypassing-scheme-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/security/contentSecurityPolicy/register-bypassing-scheme-actual.txt @@ -1,8 +1,4 @@ CONSOLE MESSAGE: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png' because it violates the following Content Security Policy directive: "img-src https:". -ALERT: PASS (1/3) -ALERT: PASS (2/3) -CONSOLE MESSAGE: Refused to load the image 'http://127.0.0.1:8000/security/resources/abe.png' because it violates the following Content Security Policy directive: "img-src https:". - -ALERT: PASS (3/3) +FAIL: Timed out waiting for notifyDone to be called This test ensures that registering a scheme as bypassing CSP actually bypasses CSP. This test passes if three PASSes are generated.
On GTK the test always passes in the retry run. Seems there's a test that's run before this one that puts the environment into a specific state in which the test then fails. On QT, in retry, it still fails, but there's another CSP test run before it[1]: http/tests/security/contentSecurityPolicy/object-src-url-allowed.html http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html ... Perhaps the test runners are not resetting something properly between each test? [1] - http://build.webkit.org/results/Qt%20Linux%20Release/r126259%20(51094)/retries/tests_run0.txt
Skip committed in r126282. Please unskip it with proper fix.
I can confirm that the test fails on GTK consistently when the test list consists of the two tests mentioned in comment #1. The test then passes when run singly. The issue when the test times out is that while the error occurs and the error timer is set to dispatch soon, the ImageLoader object gets deleted and aborts the error event firing. As to why or how the preceding test would make this happen, I have no idea.
This test now fails on Mac bots too. In debug builds, it fails with an assertion: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010963d9f9 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const + 329 (JSEventListener.h:90) 1 com.apple.WebCore 0x00000001097b5b02 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 194 (JSEventListener.cpp:80) 2 com.apple.WebCore 0x00000001091eca67 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&) + 359 (EventTarget.cpp:232) 3 com.apple.WebCore 0x00000001091ec8c5 WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 325 (EventTarget.cpp:200) 4 com.apple.WebCore 0x0000000109c9cd4b WebCore::Node::handleLocalEvents(WebCore::Event*) + 155 (Node.cpp:2546) 5 com.apple.WebCore 0x00000001091bff64 WebCore::EventContext::handleLocalEvents(WebCore::Event*) const + 276 (EventContext.cpp:55) 6 com.apple.WebCore 0x00000001091c2ab9 WebCore::EventDispatcher::dispatchEventAtTarget(WTF::PassRefPtr<WebCore::Event>) + 89 (EventDispatcher.cpp:309)
Reproducible with just run-webkit-tests -v http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html --repeat-each 20
I had a brief look at this. So, there is an error event being fired on HTMLImageElement after its listener's JavaScript wrapper has been collected. The code that tells whether it's OK to garbage collect is isReachableFromDOM() in JSNodeCustom.cpp. It basically looks at hasPendingLoadEvent(), which I think is a misnomer, as it should be covering error, too. The code hasn't been changed much in months (I think that Adam last touched it in March, to improve the naming). Mike, does CSP first stop the load, and then post an error event on a timer perhaps? That would let garbage collection perform collection too early.
(In reply to comment #6) > I had a brief look at this. So, there is an error event being fired on HTMLImageElement after its listener's JavaScript wrapper has been collected. > > The code that tells whether it's OK to garbage collect is isReachableFromDOM() in JSNodeCustom.cpp. It basically looks at hasPendingLoadEvent(), which I think is a misnomer, as it should be covering error, too. The code hasn't been changed much in months (I think that Adam last touched it in March, to improve the naming). > > Mike, does CSP first stop the load, and then post an error event on a timer perhaps? That would let garbage collection perform collection too early. Yes, the load is blocked, and then the error is fired asynchronously. I'll take a look at this issue today. Would a synchronous event trigger be acceptable? If not, perhaps I can adjust the isReachableFromDOM method to understand that an element has waiting events?
(In reply to comment #7) > I'll take a look at this issue today. Would a synchronous event trigger be acceptable? If not, perhaps I can adjust the isReachableFromDOM method to understand that an element has waiting events? Now that I'm looking at the code, the former option is stupid. :) I'll adjust hasPendingLoadEvent to check the error event sender as well. That looks straightforward.
Created attachment 163819 [details] Patch
(In reply to comment #9) > Created an attachment (id=163819) [details] > Patch Jochen, perhaps you could take a look at this before California wakes up? :)
Created attachment 163843 [details] Unskipping crashing test.
Comment on attachment 163843 [details] Unskipping crashing test. This looks correct. Let's wait whether Alexey and Adam agree.
Comment on attachment 163843 [details] Unskipping crashing test. View in context: https://bugs.webkit.org/attachment.cgi?id=163843&action=review Looks reasonable to me. I'd like to wait for Adam's comments, because (1) he was the last one to meaningfully update this code in March, and (2) his recent patch is blamed for this assertion firing elsewhere, mysteriously to me (see bug 93654). > Source/WebCore/ChangeLog:9 > + element before reclaiming it's memory. It's not, however, checking that First "it's" should be "its". > Source/WebCore/ChangeLog:20 > + This new behavior is covered by existing tests. In particular, > + http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html > + should no longer crash. The existing tests only catch this once in multiple runs, it would be good to have a test that hits the issue reliably.
> The existing tests only catch this once in multiple runs, it would be good to have a test that hits the issue reliably. You might be able to do that by explicitly calling gc() at an opportune time. There's a gc.js script somewhere in LayoutTests that makes this easy.
(In reply to comment #14) > You might be able to do that by explicitly calling gc() at an opportune time. There's a gc.js script somewhere in LayoutTests that makes this easy. FYI, fast/js/resources/js-test-pre.js has gc().
(In reply to comment #15) > (In reply to comment #14) > > You might be able to do that by explicitly calling gc() at an opportune time. There's a gc.js script somewhere in LayoutTests that makes this easy. > > FYI, fast/js/resources/js-test-pre.js has gc(). Thanks all. I'll see what I can do about reproducing the crash every time. That will be a nice change of pace from ensuring that crashes never reproduce. :)
(In reply to comment #5) > Reproducible with just > > run-webkit-tests -v http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html --repeat-each 20 I'm having issues reproducing this locally, with or without gc(). Alexey, you were reproducing it on the plain old mac port, right? Release or debug build? Nothing else interesting about the config?
(In reply to comment #17) > (In reply to comment #5) > > Reproducible with just > > > > run-webkit-tests -v http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html --repeat-each 20 > > I'm having issues reproducing this locally, with or without gc(). Alexey, you were reproducing it on the plain old mac port, right? Release or debug build? Nothing else interesting about the config? Ah. Amusing. I couldn't reproduce because I had this patch applied. :) I'll upload a new patch in a moment that GC's the heck out of register-bypassing-scheme.
Created attachment 163924 [details] Patch
Created attachment 163926 [details] it's -> its
Comment on attachment 163924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163924&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:6 > + // Copied from fast/js/resources/js-test-pre.js, which isn't available via HTTP. it's in http://127.0.0.1:8000/resources/js-test-pre.js
Created attachment 163932 [details] Jochen's feedback.
Comment on attachment 163932 [details] Jochen's feedback. Cool. Adam, Alexey, CQ?
I can't easily wrap my head around the existing nonsense code. Adam? One thing that I'm not entirely sure about is regular failing loads. Why are we not seeing this assertion failure when image load fails because of a 404 or a network issue? Is CSP different in some way - or perhaps this could be reproduced with a regular failing load, and we just horribly lack in test coverage?
Comment on attachment 163932 [details] Jochen's feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=163932&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:21 > + setTimeout(gc, 0); Doesn't this race with the image loads?
Comment on attachment 163932 [details] Jochen's feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=163932&action=review >> LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:21 >> + setTimeout(gc, 0); > > Doesn't this race with the image loads? One solution is to kick off the load in a separate function and then call gc() after returning from that function. The events should then be delivered asynchronously.
(In reply to comment #26) > (From update of attachment 163932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163932&action=review > > >> LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:21 > >> + setTimeout(gc, 0); > > > > Doesn't this race with the image loads? > > One solution is to kick off the load in a separate function and then call gc() after returning from that function. The events should then be delivered asynchronously. Something like this? (function x() { img.src = "whatever"; })(); gc(); I somehow thought that's more or less what the setTimeout(..., 0) was doing.
> Something like this? > > (function x() { img.src = "whatever"; })(); > gc(); Yes. (Note the "x" isn't needed.) > I somehow thought that's more or less what the setTimeout(..., 0) was doing. It is similar, but the difference is that in the setTimeout approach, you don't know when the gc will actually run. It gets scheduled on the event loop, but it might happen before or after the load and/or error tests happen.
Created attachment 164052 [details] Function.
(In reply to comment #28) > > Something like this? > > > > (function x() { img.src = "whatever"; })(); > > gc(); > > Yes. (Note the "x" isn't needed.) > > > I somehow thought that's more or less what the setTimeout(..., 0) was doing. > > It is similar, but the difference is that in the setTimeout approach, you don't know when the gc will actually run. It gets scheduled on the event loop, but it might happen before or after the load and/or error tests happen. Thanks for the explanation. That makes sense. The newly attached patch implements the suggestion.
Comment on attachment 164052 [details] Function. View in context: https://bugs.webkit.org/attachment.cgi?id=164052&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:21 > + var img = document.createElement('img'); > + img.onload = function () { > + alert('FAIL (1/1)'); > + finishTesting(); > + }; > + img.onerror = function () { > + alert('PASS (1/1)'); > + finishTesting(); > + }; > + (function () { img.src = "../resources/abe.png"; })(); > + gc(); This won't work quite right. You need to put all references to img inside the function so that img will be garbage by the time you call gc(). Does this test successfully replicate the failure?
(In reply to comment #24) > I can't easily wrap my head around the existing nonsense code. Adam? > > One thing that I'm not entirely sure about is regular failing loads. Why are we not seeing this assertion failure when image load fails because of a 404 or a network issue? Is CSP different in some way - or perhaps this could be reproduced with a regular failing load, and we just horribly lack in test coverage? On IRC, Jochen proposed that we add at least one more test to this patch to check this case. I'll take care of that sometime in the vaguely near future, along with Adam's note from below. Thanks!
Created attachment 164198 [details] Hrm.
(In reply to comment #32) > On IRC, Jochen proposed that we add at least one more test to this patch to check this case. I'll take care of that sometime in the vaguely near future, along with Adam's note from below. I've added a test similar to what I'd talked about with Jochen, but I couldn't get it to crash, with or without the patch. That's good, I suppose! Or, it's proof that I misunderstood Jochen's suggestion. Mind taking a look?
Comment on attachment 164198 [details] Hrm. View in context: https://bugs.webkit.org/attachment.cgi?id=164198&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html:27 > + if (window.testRunner) { No need for { } in this if
Interesting. I'm inclined to say we should land this patch with the extra test. @jochen: What do you think?
Created attachment 164223 [details] Fewer curly bits.
(In reply to comment #36) > Interesting. I'm inclined to say we should land this patch with the extra test. @jochen: What do you think? Cool. Uploaded a final(?) patch without the superfluous braces. If you're happy with the test Jochen, CQ?
Comment on attachment 164223 [details] Fewer curly bits. Clearing flags on attachment: 164223 Committed r128730: <http://trac.webkit.org/changeset/128730>
All reviewed patches have been landed. Closing bug.