[V8] Notification.requestPermission(function() {alert();}) crashes

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Kentaro Hara]

Notification.requestPermission(function() {alert();}) crashes. See the Chromium bug for more details: http://code.google.com/p/chromium/issues/detail?id=143490

[Kentaro Hara]

Created attachment 159392 [details]
Patch

[Jon Lee]

This patch looks ok. A few comments about testing.

We probably should use the js-test-{pre,post-async}.js for the test, yes?

I have a rather large patch which I'm trying to break up to bring notifications support to Mac. As part of this, I wrote some tests, but put them in http/tests/notifications. I think most new tests should be placed there, since we can leverage the origin in http/tests. It may be possible to rewrite this test in there so that you don't have to do it manually. (As part of that, you'd have to copy the js-test-*.js files into http/tests/resources.)

Otherwise, I'd rather have this test in ManualTests instead.

I would r+, but am not a reviewer.

[Adam Barth]

Comment on attachment 159392 [details]
Patch

Please address Jon's feedback before landing.

[Kentaro Hara]

(In reply to comment #2)
> I have a rather large patch which I'm trying to break up to bring notifications support to Mac. As part of this, I wrote some tests, but put them in http/tests/notifications. I think most new tests should be placed there, since we can leverage the origin in http/tests. It may be possible to rewrite this test in there so that you don't have to do it manually. (As part of that, you'd have to copy the js-test-*.js files into http/tests/resources.)
> 
> Otherwise, I'd rather have this test in ManualTests instead.

What is best for your upcoming patch? May I create http/tests/notifications and add the test to http/tests/notifications? Or, given that the test is anyway not testable in Chrome due to bug 81697, can I just put it to ManualTests? Either is fine for me.

[Jon Lee]

(In reply to comment #4)
> (In reply to comment #2)
> > I have a rather large patch which I'm trying to break up to bring notifications support to Mac. As part of this, I wrote some tests, but put them in http/tests/notifications. I think most new tests should be placed there, since we can leverage the origin in http/tests. It may be possible to rewrite this test in there so that you don't have to do it manually. (As part of that, you'd have to copy the js-test-*.js files into http/tests/resources.)
> > 
> > Otherwise, I'd rather have this test in ManualTests instead.
> 
> What is best for your upcoming patch? May I create http/tests/notifications and add the test to http/tests/notifications? Or, given that the test is anyway not testable in Chrome due to bug 81697, can I just put it to ManualTests? Either is fine for me.

Add your test to http/tests/notifications, with the js-test-*.js files add to resources/. The test will work in http/tests because the URL will be http://127.0.0.1/, not file://. So you'll need to change the permission grant to add "http://127.0.0.1".

Once I land my patches, I can refactor that test as needed. (For example, I think we should use the term WebNotifications instead of DesktopNotifications, etc.). This would mean you'll need to add it to Skipped for Mac, and any other ports that don't support notifications tests.

[Kentaro Hara]

(In reply to comment #5)
> Add your test to http/tests/notifications, with the js-test-*.js files add to resources/. The test will work in http/tests because the URL will be http://127.0.0.1/, not file://. So you'll need to change the permission grant to add "http://127.0.0.1".

Sorry, would you tell me how to grant the permission to 127.0.0.1?

The following test outputs nothing even if I put it in http/tests/notifications.

  if (!window.Notification)
    testFailed("window.Notification does not exist.");
  window.Notification.requestPermission(function() {
    testPassed("Permission callback invoked.");
  });

[Jon Lee]

(In reply to comment #6)
> (In reply to comment #5)
> > Add your test to http/tests/notifications, with the js-test-*.js files add to resources/. The test will work in http/tests because the URL will be http://127.0.0.1/, not file://. So you'll need to change the permission grant to add "http://127.0.0.1".
> 
> Sorry, would you tell me how to grant the permission to 127.0.0.1?
> 
> The following test outputs nothing even if I put it in http/tests/notifications.
> 
>   if (!window.Notification)
>     testFailed("window.Notification does not exist.");
>   window.Notification.requestPermission(function() {
>     testPassed("Permission callback invoked.");
>   });

The function gets called asynchronously. So I think you need a waitUntilDone() and a notifyDone().

[Kentaro Hara]

Created attachment 160130 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 160130 [details]
patch for landing

Rejecting attachment 160130 [details] from commit-queue.

New failing tests:
http/tests/notifications/notification-request-permission.html
Full output: http://queues.webkit.org/results/13566634

[WebKit Review Bot]

Created attachment 160138 [details]
Archive of layout-test-results from gce-cq-04

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Jon Lee]

Patch is missing a lot of files, but maybe you wanted to see what EWS returned.

Suggestion for the test: look at fast/css/image-resolution/image-resolution.html, which uses js-test-post-async.js, instead of js-test-post.js. So it would be something like:

window.Notification.requestPermission(function() {
alert();
testPassed("Notification.requestPermission does not crash.");
testCompleted();
});

...


function testCompleted()
{
    var scriptElement = document.createElement("script");
    scriptElement.src = '../resources/js-test-post-async.js';
    document.body.appendChild(scriptElement);
}

[Kentaro Hara]

Created attachment 164641 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 164641 [details]
patch for landing

Clearing flags on attachment: 164641

Committed r128959: <http://trac.webkit.org/changeset/128959>

[Anders Carlsson]

V8 is gone.