WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
94462
[V8] Notification.requestPermission(function() {alert();}) crashes
https://bugs.webkit.org/show_bug.cgi?id=94462
Summary
[V8] Notification.requestPermission(function() {alert();}) crashes
Kentaro Hara
Reported
2012-08-20 03:58:30 PDT
Notification.requestPermission(function() {alert();}) crashes. See the Chromium bug for more details:
http://code.google.com/p/chromium/issues/detail?id=143490
Attachments
Patch
(5.53 KB, patch)
2012-08-20 04:22 PDT
,
Kentaro Hara
abarth
: commit-queue-
Details
Formatted Diff
Diff
patch for landing
(3.43 KB, patch)
2012-08-23 04:57 PDT
,
Kentaro Hara
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cq-04
(376.53 KB, application/zip)
2012-08-23 05:40 PDT
,
WebKit Review Bot
no flags
Details
patch for landing
(3.92 KB, patch)
2012-09-18 18:12 PDT
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Kentaro Hara
Comment 1
2012-08-20 04:22:07 PDT
Created
attachment 159392
[details]
Patch
Jon Lee
Comment 2
2012-08-20 09:05:00 PDT
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 3
2012-08-20 11:46:54 PDT
Comment on
attachment 159392
[details]
Patch Please address Jon's feedback before landing.
Kentaro Hara
Comment 4
2012-08-20 17:42:10 PDT
(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
Comment 5
2012-08-21 18:03:15 PDT
(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
Comment 6
2012-08-21 21:35:23 PDT
(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
Comment 7
2012-08-21 22:28:34 PDT
(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
Comment 8
2012-08-23 04:57:00 PDT
Created
attachment 160130
[details]
patch for landing
WebKit Review Bot
Comment 9
2012-08-23 05:40:30 PDT
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
Comment 10
2012-08-23 05:40:32 PDT
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
Comment 11
2012-08-23 08:47:45 PDT
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
Comment 12
2012-09-18 18:12:37 PDT
Created
attachment 164641
[details]
patch for landing
WebKit Review Bot
Comment 13
2012-09-18 18:36:01 PDT
Comment on
attachment 164641
[details]
patch for landing Clearing flags on attachment: 164641 Committed
r128959
: <
http://trac.webkit.org/changeset/128959
>
Anders Carlsson
Comment 14
2013-09-12 22:35:25 PDT
V8 is gone.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug