Bug 99658 – Crash when trying to write exception message to null console

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Toni Barzic]

Crash when trying to write exception message to null console

[Toni Barzic]

Created attachment 169301 [details]
Patch

[Toni Barzic]

Chromium issue tracked at http://crbug.com/149054

the crash may happen e.g. if a worker throws an exception just as the document is being detached from the frame (and before the worker's script execution context is deleted).

I don't really have an idea how to generate test case for this (at least a non-flaky one), suggestions welcomed :)


I was able to repro the problem in Chromium using following example:
************************************************************************

--- repro.html

<script>
var worker = new Worker('repro_worker.js');
setTimeout(function() { document.location.reload(true) }, 100);
</script>

--- repro_worker.js

setTimeout(function(){ throw Error('ERRRRRRRRRRRRR') }, 80);

****************************************************************************

[Adam Barth]

Comment on attachment 169301 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> +
> +        No new tests (OOPS!).

Please fill out these parts of the ChangeLog.  Also, please add a test that triggers this issue.

[Toni Barzic]

Created attachment 169719 [details]
Patch

[Toni Barzic]

Created attachment 169743 [details]
Patch

[Toni Barzic]

Created attachment 169745 [details]
Patch

[Adam Barth]

Comment on attachment 169745 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        

You've got an extra blank line here.

> Source/WebCore/ChangeLog:13
> +

This blank line is also extra.

> LayoutTests/ChangeLog:3
> +        Regression test for 

Usually we just have the bug title here, like in the WebCore ChangeLog entry

> LayoutTests/ChangeLog:10
> +        Unfortuantelly, the test is inherently flaky and may produce some false positive results (but should never fail if there is no bug).

typo: Unfortuantelly

> LayoutTests/fast/workers/worker-exception-during-navigation.html:10
> +function gc()
> +{
> +    if (window.GCController)
> +        return GCController.collect();
> +
> +    for (var i = 0; i < 10000; i++) { // > force garbage collection (FF requires about 9K allocations before a collect)
> +        var s = new String("abc");
> +    }
> +}

Please use the gc.js file that has this logic rather than copy/pasting it here.

> LayoutTests/fast/workers/worker-exception-during-navigation.html:19
> +        }, 100);

Why 100?

> LayoutTests/fast/workers/worker-exception-during-navigation.html:32
> +    for (var i = 0; i< 500; i++) {

500 workers!  Oh my.

[Adam Barth]

Comment on attachment 169745 [details]
Patch

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

Ok.  this makes sense.  Thanks for writing the test case.  The test case is kind of nutty, but I guess that's needed since the bug is inherently racy.

>> LayoutTests/fast/workers/worker-exception-during-navigation.html:32
>> +    for (var i = 0; i< 500; i++) {
> 
> 500 workers!  Oh my.

Also, we're missing a space before the <

[Toni Barzic]

Created attachment 169966 [details]
Patch

[Toni Barzic]

Comment on attachment 169745 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        
> 
> You've got an extra blank line here.

Done.

>> Source/WebCore/ChangeLog:13
>> +
> 
> This blank line is also extra.

Done.

>> LayoutTests/ChangeLog:3
>> +        Regression test for 
> 
> Usually we just have the bug title here, like in the WebCore ChangeLog entry

Done.

>> LayoutTests/ChangeLog:10
>> +        Unfortuantelly, the test is inherently flaky and may produce some false positive results (but should never fail if there is no bug).
> 
> typo: Unfortuantelly

Done.

>> LayoutTests/fast/workers/worker-exception-during-navigation.html:10
>> +}
> 
> Please use the gc.js file that has this logic rather than copy/pasting it here.

Done.

>> LayoutTests/fast/workers/worker-exception-during-navigation.html:19
>> +        }, 100);
> 
> Why 100?

To give the test enough time to crash. Added comment.

>>> LayoutTests/fast/workers/worker-exception-during-navigation.html:32
>>> +    for (var i = 0; i< 500; i++) {
>> 
>> 500 workers!  Oh my.
> 
> Also, we're missing a space before the <

yeah, I reduced the number of workers (I planned to do this all along, but didn't manage on Friday)

the test is still kinda funky, but I can't think of a better way to test this :(

[Adam Barth]

Comment on attachment 169966 [details]
Patch

Ok.  I'm not super excited about this test, but I don't have a better suggestion for you.

[WebKit Review Bot]

Comment on attachment 169966 [details]
Patch

Rejecting attachment 169966 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 169966 [details]
Patch

Clearing flags on attachment: 169966

Committed r132371: <http://trac.webkit.org/changeset/132371>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.