Bug 94677 – http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html fails, triggering an assertion in JSEventListener::jsFunction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Balazs Ankes]

--- /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.

[Zan Dobersek]

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

[Balazs Ankes]

Skip committed in r126282. Please unskip it with proper fix.

[Zan Dobersek]

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.

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

Reproducible with just 

run-webkit-tests -v http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html --repeat-each 20

[Alexey Proskuryakov]

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.

[Mike West]

(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?

[Mike West]

(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.

[Mike West]

Created attachment 163819 [details]
Patch

[Mike West]

(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? :)

[Mike West]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

> 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.

[Kentaro Hara]

(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().

[Mike West]

(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. :)

[Mike West]

(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?

[Mike West]

(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.

[Mike West]

Created attachment 163924 [details]
Patch

[Mike West]

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

[Mike West]

Created attachment 163932 [details]
Jochen's feedback.

[Mike West]

Comment on attachment 163932 [details]
Jochen's feedback.

Cool. Adam, Alexey, CQ?

[Alexey Proskuryakov]

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?

[Adam Barth]

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?

[Adam Barth]

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.

[Mike West]

(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.

[Adam Barth]

> 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.

[Mike West]

Created attachment 164052 [details]
Function.

[Mike West]

(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.

[Adam Barth]

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?

[Mike West]

(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!

[Mike West]

Created attachment 164198 [details]
Hrm.

[Mike West]

(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?

[Adam Barth]

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

[Adam Barth]

Interesting.  I'm inclined to say we should land this patch with the extra test.  @jochen: What do you think?

[Mike West]

Created attachment 164223 [details]
Fewer curly bits.

[Mike West]

(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?

[WebKit Review Bot]

Comment on attachment 164223 [details]
Fewer curly bits.

Clearing flags on attachment: 164223

Committed r128730: <http://trac.webkit.org/changeset/128730>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.