Bug 31721

Summary:

Using jQuery to show/hide IMG elements crashes WebKit

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

Images

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andrey.petrov, ap, aroben, commit-queue, darin, dbates, dglazkov, enrica, jberlin, joone, mitz, rniwa, simon.fraser, webkit.review.bot, yael

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Archive of the javascript application	none
Crash log on Mac	none
Dr watson log on WinXP	none
User dmp on WinXP	none
Simple HTML test file, no jquery involved	none
proposed patch	webkit-ews: commit-queue-
trival patch	none
proposed patch, take 3	webkit-ews: commit-queue-
trivial patch	dbates: review-, dbates: commit-queue-
trivial patch	dbates: review-, dbates: commit-queue-
new patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03	none
new patch	abarth: review-, abarth: commit-queue-
new version	rniwa: commit-queue-
next version	none
new version	morrita: review+, commit-queue: commit-queue-
new version	none

Mihnea Ovidenie

Reported 2009-11-20 06:10:11 PST

Created attachment 43574 [details] Archive of the javascript application Hi, I was using JQuery 1.3.2 to show/hide IMG elements in a simple javascript application. <html> <head> <script type="text/javascript" src="jquery-1.3.2.js"></script> <script> // this reproduces the problem because hide calls animate function openPreferences() { $('#DIV').hide ( 300, function() { $('#DIV_NIGHT').show(300); } ); } </script> </head> <body onmousedown="openPreferences()"> <div> <div ID="DIV"> <div> <img id="IMG" src="5day.png"/> </div> </div> <div ID="DIV_NIGHT" style="display:none"> <div> <img id="IMG_NIGHT" src="night.png"/> </div> </div> </div> Test </body> </html> 1. When the application is displayed, i place the mouse over the image displayed (5day.png). 2. Right-click 3. From the contextual menu opened, i choose "Copy Image". Note that in the meantime, the original image is replaced with another image. 4. As an effect of step 3 - context menu & image replaced, the WebKit browser crashes The problem appears on Mac 10.5.8 Safari 4.0.4/WebKit 51229 WindowsXP Safari 4.0.4/WebKit 51228. Chrome3.0.195.27 does not crash (Win), FF3.5.4 does not crash (Mac/Win), Opera 10 does not crash (Mac/Win), IE8 does not crash on Win. Digging a little in the code: 1. In WebCore/editing/Editor.cpp copyImage(const HitTestResult& result) The member m_innerNonSharedNode from HitTestResult does not have a valid renderer (0). Because of that, absoluteImageURL() returns a url that is not valid and cannot be used to be passed to writeImage. 2. A possible fix to this issue would be to check the url to be valid before calling writeImage if (url.isValid()) Pasteboard::generalPasteboard()->writeImage(result.innerNonSharedNode(), url, result.altDisplayString()); However, i do not understand the exact cause of the issue, so the above fix might not be very appropriate. Could be related to WebKit bug 25381: jQuery animation crashing Safari (Mac/Win) / Chrome (Win) Regards, Mihnea

Attachments
Archive of the javascript application (41.26 KB, application/zip) 2009-11-20 06:10 PST, Mihnea Ovidenie	no flags	Details
Crash log on Mac (35.42 KB, text/plain) 2009-11-20 06:11 PST, Mihnea Ovidenie	no flags	Details
Dr watson log on WinXP (138.85 KB, application/octet-stream) 2009-11-20 06:13 PST, Mihnea Ovidenie	no flags	Details
User dmp on WinXP (57.10 KB, application/octet-stream) 2009-11-20 06:14 PST, Mihnea Ovidenie	no flags	Details
Simple HTML test file, no jquery involved (576 bytes, text/html) 2009-11-25 07:38 PST, Mihnea Ovidenie	no flags	Details
proposed patch (4.71 KB, patch) 2011-05-18 11:30 PDT, Andrey Petrov	webkit-ews: commit-queue-	Details Formatted Diff Diff
trival patch (4.72 KB, patch) 2011-05-18 12:01 PDT, Andrey Petrov	no flags	Details Formatted Diff Diff
proposed patch, take 3 (4.71 KB, patch) 2011-05-18 16:15 PDT, Andrey Petrov	webkit-ews: commit-queue-	Details Formatted Diff Diff
trivial patch (4.72 KB, patch) 2011-05-18 16:26 PDT, Andrey Petrov	dbates: review- dbates: commit-queue-	Details Formatted Diff Diff
trivial patch (6.05 KB, patch) 2011-05-18 16:47 PDT, Andrey Petrov	dbates: review- dbates: commit-queue-	Details Formatted Diff Diff
new patch (9.11 KB, patch) 2011-05-19 21:33 PDT, Andrey Petrov	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.50 MB, application/zip) 2011-05-19 21:49 PDT, WebKit Review Bot	no flags	Details
new patch (9.18 KB, patch) 2011-05-19 22:15 PDT, Andrey Petrov	abarth: review- abarth: commit-queue-	Details Formatted Diff Diff
new version (8.99 KB, patch) 2011-05-20 20:01 PDT, Andrey Petrov	rniwa: commit-queue-	Details Formatted Diff Diff
next version (9.18 KB, patch) 2011-05-20 21:56 PDT, Andrey Petrov	no flags	Details Formatted Diff Diff
new version (deleted) 2011-05-20 22:08 PDT, Andrey Petrov	morrita: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
new version (8.86 KB, patch) 2011-05-27 11:08 PDT, Andrey Petrov	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Mihnea Ovidenie

Comment 1 2009-11-20 06:11:11 PST

Created attachment 43575 [details] Crash log on Mac

Mihnea Ovidenie

Comment 2 2009-11-20 06:13:26 PST

Created attachment 43576 [details] Dr watson log on WinXP

Mihnea Ovidenie

Comment 3 2009-11-20 06:14:22 PST

Created attachment 43577 [details] User dmp on WinXP

Alexey Proskuryakov

Comment 4 2009-11-20 12:56:07 PST

<rdar://problem/7413760>

Mihnea Ovidenie

Comment 5 2009-11-25 07:38:44 PST

Created attachment 43844 [details] Simple HTML test file, no jquery involved I made a simplified version of the crash and tested with latest nighlty 51303 on Windows. This time, no jquery involved. Basically, the code that crashes WebKit is as follows: <script> function openPreferences() { setTimeout(function() { document.getElementById('DIV').style.display = 'none'; }, 100); } </script> <body onmousedown="openPreferences()"> <div> <div ID="DIV"> <div> <img id="IMG" src="5day.png"/> </div> </div> </div> </body> If hiding (or removing) the image from DOM is done on a timer, then the right click menu can be displayed and in addition to that, when the CopyImage action is chosen, Webkit crashes since the renderer for the node is 0. Regards, Mihnea

Andrey Petrov

Comment 6 2011-05-18 10:56:55 PDT

I've tried this on windows and linux/gtk and it crashes all the same. The reason is indeed renderer set to NULL as the detachment already happened. I tried to reproduce this issue (copying image of a node into clipboard) with some other trigger (.removeChild on button handler) and naturally it crashes the same way. I think the right way to fix this issue would be to remove superfluous ASSERTs and if renderer absents just exit gracefully. I grepped code all around and in many places existence of a renderer for a given node is not assumed and a fallback is implemented. Moreover Android version already checks if render is 0 so it would appear to be the right way. I tried to reproduce a case where the NODE actually could be deleted by the time action is activated. I did not succeed no matter how I tried to kill the node with javascript or even with meta refresh. So I think it is safe to leave assertion for node.

Andrey Petrov

Comment 7 2011-05-18 11:30:52 PDT

Created attachment 93951 [details] proposed patch proposed trivial patch for gtk, chromium, qt, win and WinCE

Early Warning System Bot

Comment 8 2011-05-18 11:56:10 PDT

Comment on attachment 93951 [details] proposed patch Attachment 93951 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8705774

Andrey Petrov

Comment 9 2011-05-18 12:01:13 PDT

Created attachment 93960 [details] trival patch trivial patch, fixed a build error;)

Andrey Petrov

Comment 10 2011-05-18 16:15:24 PDT

Created attachment 93997 [details] proposed patch, take 3 added Mac

Early Warning System Bot

Comment 11 2011-05-18 16:24:46 PDT

Comment on attachment 93997 [details] proposed patch, take 3 Attachment 93997 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8710644

Andrey Petrov

Comment 12 2011-05-18 16:26:42 PDT

Created attachment 93998 [details] trivial patch again broken Qt and fixed it again

Daniel Bates

Comment 13 2011-05-18 16:29:04 PDT

Comment on attachment 93998 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=93998&action=review This bug still affects Mac Safari as of version 5.0.5 (6533.21.1, r86784). But I don't see any changes in this patch for the Mac port. > Source/WebCore/ChangeLog:8 > + No new tests. Can you elaborate on why we can't test this? It seems like we could at least have a manual test case. > Source/WebCore/ChangeLog:10 > + For platform code, do not assert nodes always have renderer. It would be beneficial if you could elaborate a bit on this.

Andrey Petrov

Comment 14 2011-05-18 16:32:33 PDT

(In reply to comment #13) thanks for looking over this. > Can you elaborate on why we can't test this? It seems like we could at least > have a manual test case. Yeah this is pretty easily testable. The test case is actually available in this bug. I thought only automated test cases can be mentioned. Will fix it. > > > Source/WebCore/ChangeLog:10 > > + For platform code, do not assert nodes always have renderer. > > It would be beneficial if you could elaborate a bit on this. please look at comment #6. If the reasoning sounds good I can of course paste it into the changelog.

Andrey Petrov

Comment 15 2011-05-18 16:33:37 PDT

(In reply to comment #13) > This bug still affects Mac Safari as of version 5.0.5 (6533.21.1, r86784). > But I don't see any changes in this patch for the Mac port. yeah I already noticed it. The latest patch fixes Mac too

Ryosuke Niwa

Comment 16 2011-05-18 16:38:06 PDT

(In reply to comment #14) > (In reply to comment #13) > > thanks for looking over this. > > > Can you elaborate on why we can't test this? It seems like we could at least > > have a manual test case. > > Yeah this is pretty easily testable. The test case is actually available in this bug. I thought only automated test cases can be mentioned. Will fix it. The attachment 43844 [details] can definitely be automated by eventSender.

Andrey Petrov

Comment 17 2011-05-18 16:47:07 PDT

Created attachment 94002 [details] trivial patch now added Mac for real, elaborated changelog.

Daniel Bates

Comment 18 2011-05-18 16:54:22 PDT

Comment on attachment 94002 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=94002&action=review > Source/WebCore/ChangeLog:7 > + The isolated testcase can be found at the bug: > + https://bugs.webkit.org/show_bug.cgi?id=31721#c5 This is insufficient. The test case should be included in the patch. As mentioned by Ryosuke Niwa, we can write a test case for this bug using DRT and the EventSender object. One such example that uses EventSender to click a context menu item is <http://trac.webkit.org/browser/trunk/LayoutTests/media/context-menu-actions.html>. > Source/WebCore/ChangeLog:189 > +>>>>>>> .r86799 Merge conflict marker.

Daniel Bates

Comment 19 2011-05-18 17:26:14 PDT

Comment on attachment 94002 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=94002&action=review > Source/WebCore/platform/mac/PasteboardMac.mm:297 > ASSERT(cocoaURL); Wouldn't cocoaURL be nil here since Editor::copyImage() would pass an empty KURL()? Hence, an assertion failure.

Andrey Petrov

Comment 20 2011-05-19 21:28:07 PDT

Comment on attachment 94002 [details] trivial patch View in context: https://bugs.webkit.org/attachment.cgi?id=94002&action=review >> Source/WebCore/platform/mac/PasteboardMac.mm:297 >> ASSERT(cocoaURL); > > Wouldn't cocoaURL be nil here since Editor::copyImage() would pass an empty KURL()? Hence, an assertion failure. hm, looks like you are right. I am reversing the order so that this does not happen

Andrey Petrov

Comment 21 2011-05-19 21:33:44 PDT

Created attachment 94169 [details] new patch new patch. this time with a brand new layout test.

WebKit Review Bot

Comment 22 2011-05-19 21:49:52 PDT

Comment on attachment 94169 [details] new patch Attachment 94169 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8721200 New failing tests: editing/pasteboard/copy-standalone-image-crash.html

WebKit Review Bot

Comment 23 2011-05-19 21:49:59 PDT

Created attachment 94171 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Andrey Petrov

Comment 24 2011-05-19 22:15:06 PDT

Created attachment 94174 [details] new patch this time wont break cr-linux hopefully

Andrey Petrov

Comment 25 2011-05-20 05:02:36 PDT

Comment on attachment 94174 [details] new patch looks like bots are happy. Asking for review

Adam Barth

Comment 26 2011-05-20 11:13:19 PDT

Comment on attachment 94174 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=94174&action=review > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:8 > + layoutTestController.dumpEditingCallbacks(); What's the point of dumpEditingCallbacks if you're just going to overwrite it all with PASS in the end? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 > + setTimeout(doctxmenu, 10); > + setTimeout(hideframe, 3000); Do we need to use setTimeout? That makes tests slow and flaky. Are there some events we should wait for instead? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:65 > + layoutTestController.waitUntilDone(); This should be in the if-block at the top of the page.

Adam Barth

Comment 27 2011-05-20 11:13:42 PDT

Comment on attachment 94174 [details] new patch The code change looks good, but the test probably needs another iteration.

Andrey Petrov

Comment 28 2011-05-20 19:57:52 PDT

Comment on attachment 94174 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=94174&action=review >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:8 >> + layoutTestController.dumpEditingCallbacks(); > > What's the point of dumpEditingCallbacks if you're just going to overwrite it all with PASS in the end? agreed, this is pretty dumb >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 >> + setTimeout(hideframe, 3000); > > Do we need to use setTimeout? That makes tests slow and flaky. Are there some events we should wait for instead? the bigger 3s timeout is for manual testing. For automation at least one timeout is needed. Anyway I reduced amount of timeouts and now with the automation there is only on 10ms timeout. I hope that is good. The tests executes at 0.4s on my box

Andrey Petrov

Comment 29 2011-05-20 20:01:52 PDT

Created attachment 94308 [details] new version Reduced timeouts usage in automated script, now gross exec time is down to 0.4s. Fixed other suggestions.

Ryosuke Niwa

Comment 30 2011-05-20 21:06:55 PDT

Comment on attachment 94308 [details] new version View in context: https://bugs.webkit.org/attachment.cgi?id=94308&action=review I'm sorry but you'd have to polish up your test. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:1 > +<html> Missing <!DOCTYPE html> > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:12 > +function doclick() { camelCase. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:13 > + Why does the every function start with a blank line? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:23 > + // it is pretty tricky to deal with accelerators in a decent way, but I suppose > + // just removing the underscores is a good enough way > + I don't think this comment adds any useful information. Please remove it. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:32 > + // As long as didn't crash, we passed. This comment is redundant. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:38 > +function hidediv() { camelCase. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:41 > + var div = document.getElementById ("DIV"); > + div.style.display="none"; No need for a temporary div. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:56 > + if (!window.layoutTestController) > + return; > + Wrong indentation level. > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 > + items = eventSender.contextClick(); What's the point in calling contextClick() here? > LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:67 > + setTimeout(doclick, 10); What's the point of this setTimeout?

Andrey Petrov

Comment 31 2011-05-20 21:14:18 PDT

Comment on attachment 94308 [details] new version View in context: https://bugs.webkit.org/attachment.cgi?id=94308&action=review >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:32 >> + // As long as didn't crash, we passed. > > This comment is redundant. copied it from some other tests (found it useful actually), but sure >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:63 >> + items = eventSender.contextClick(); > > What's the point in calling contextClick() here? to popup context menu and to programmatically learn action items available >> LayoutTests/editing/pasteboard/copy-standalone-image-crash.html:67 >> + setTimeout(doclick, 10); > > What's the point of this setTimeout? to execute detachment of render to the node WHILE the context menu is popped up. This is the heart of the problem in this bug

Andrey Petrov

Comment 32 2011-05-20 21:56:57 PDT

Created attachment 94317 [details] next version the new version hopefully addresses Ryosuke's comments.

Andrey Petrov

Comment 33 2011-05-20 22:06:55 PDT

Comment on attachment 94317 [details] next version forgot doctype

Andrey Petrov

Comment 34 2011-05-20 22:08:51 PDT

Created attachment 94319 [details] new version the once again new version. if I polish it more I probably would tear the paint ;)

Hajime Morrita

Comment 35 2011-05-24 20:48:44 PDT

Comment on attachment 94319 [details] new version Why not add this guard to caller side? Pasteboard method already has so many code duplication between ports. I don't want more.

Andrey Petrov

Comment 36 2011-05-24 22:27:20 PDT

(In reply to comment #35) > (From update of attachment 94319 [details]) > Why not add this guard to caller side? Pasteboard method already has so many code duplication between ports. > I don't want more. yeah, but code duplication in pasteboard is unavoidable. If it is designed that way we just have to live with it or change design

Mihnea Ovidenie

Comment 37 2011-05-24 22:45:14 PDT

(In reply to comment #36) > (In reply to comment #35) > > (From update of attachment 94319 [details] [details]) > > Why not add this guard to caller side? Pasteboard method already has so many code duplication between ports. > > I don't want more. > > yeah, but code duplication in pasteboard is unavoidable. If it is designed that way we just have to live with it or change design Did you miss an assertion on the Gtk port for: ASSERT(node->renderer()->isImage()); From that code alone, it appears so. This is why i like the above comment from Morita. Pasteboard needs some refactoring and this bug may be a good start in this direction. Mihnea

Andrey Petrov

Comment 38 2011-05-25 01:43:39 PDT

(In reply to comment #37) > Did you miss an assertion on the Gtk port for: > ASSERT(node->renderer()->isImage()); > > From that code alone, it appears so. This is why i like the above comment from Morita. Pasteboard needs some refactoring and this bug may be a good start in this direction. No, I did not miss it. Some platform code asserts isImage(), some doesn't. I thought the idea of this bug was just to fix a crash. Refactoring should be done under different bugs in my opinion. Now then, shall I just move the check to the caller?

Hajime Morrita

Comment 39 2011-05-25 18:54:29 PDT

Comment on attachment 94319 [details] new version Andrey's claim makes sense: The bug fix and refactoring should have separate patches. So let's fix it, and start refactoring!

WebKit Commit Bot

Comment 40 2011-05-25 21:27:17 PDT

The commit-queue encountered the following flaky tests while processing attachment 94319 [details]: animations/suspend-resume-animation.html bug 48161 (authors: cmarrin@apple.com and simon.fraser@apple.com) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 41 2011-05-25 21:28:37 PDT

Comment on attachment 94319 [details] new version Rejecting attachment 94319 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1 Last 500 characters of output: ource/JavaScriptCore/assembler/AbstractMacroAssembler.h M Source/JavaScriptCore/assembler/LinkBuffer.h M Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h M Source/JavaScriptCore/ChangeLog r87356 = 0d15b9e08d45889b062418435b04bb482fb278a8 (refs/remotes/trunk) M LayoutTests/platform/qt/Skipped M LayoutTests/ChangeLog r87357 = 222d6ccfdb66b9a5e75f5b5d5a1ef79f89c91769 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8733800

Andrey Petrov

Comment 42 2011-05-25 21:38:57 PDT

Morrita, thanks. Looks like you need to modify changelog: ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Darin Adler

Comment 43 2011-05-26 11:57:18 PDT

Comment on attachment 94319 [details] new version If the renderer can be gone, then I expect it can also be a non-image renderer due to changes in the CSS content attribute. So I think that other assertion, the one about isImage, is also incorrect.

Andrey Petrov

Comment 44 2011-05-26 13:35:41 PDT

(In reply to comment #43) > (From update of attachment 94319 [details]) > If the renderer can be gone, then I expect it can also be a non-image renderer due to changes in the CSS content attribute. So I think that other assertion, the one about isImage, is also incorrect. Darin, do you think you can hack together a simple javascript/css testcase?

Daniel Bates

Comment 45 2011-05-26 14:44:55 PDT

CC'ing Darin Adler since Andrey Petrov responded to him in comment 44.

Darin Adler

Comment 46 2011-05-26 14:55:31 PDT

No, sorry, I don’t have time to work on this personally to test my guess at the moment due to my work load.

Andrey Petrov

Comment 47 2011-05-26 14:57:42 PDT

Ok thanks. Then we can a file new bug later for this if we do run into a case when it triggers. Perhaps this can be moved to caller as well when we do the refactoring already mentioned.

Darin Adler

Comment 48 2011-05-26 15:07:19 PDT

(In reply to comment #47) > Then we can a file new bug later for this if we do run into a case when it triggers. Or we can make code that doesn’t just assert something because it wishes for it. It’s best to have reproducible cases for bugs to fix, but we also can write code that doesn’t make assertions for things that can’t be guaranteed.

Andrey Petrov

Comment 49 2011-05-26 15:44:43 PDT

So do you Darin think I should nuke the isImage() asserts then? (I am not expert in this and I a bit cautious about making changes which I can not reproduce)

Darin Adler

Comment 50 2011-05-26 16:37:50 PDT

(In reply to comment #49) > So do you Darin think I should nuke the isImage() asserts then? No, I think we should change the isImage asserts into actual isImage runtime checks.

Andrey Petrov

Comment 51 2011-05-27 11:08:40 PDT

Created attachment 95192 [details] new version new version also adds isImage() runtime check as suggested

Andrey Petrov

Comment 52 2011-05-27 14:11:33 PDT

Comment on attachment 95192 [details] new version bots seem to be happy. asking for review

Hajime Morrita

Comment 53 2011-05-30 20:57:02 PDT

Comment on attachment 95192 [details] new version let us try again.

WebKit Commit Bot

Comment 54 2011-05-30 22:01:04 PDT

Comment on attachment 95192 [details] new version Clearing flags on attachment: 95192 Committed r87709: <http://trac.webkit.org/changeset/87709>

WebKit Commit Bot

Comment 55 2011-05-30 22:01:16 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 56 2011-06-02 16:28:16 PDT

The test is failing on Windows port: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r87953%20(13385)/editing/pasteboard/copy-standalone-image-crash-pretty-diff.html Should it be skipped?

Andrey Petrov

Comment 57 2011-06-02 21:26:29 PDT

hm looks like this is a limitation of eventsender, the following code does not seem to work on windows: actionitems = eventSender.contextClick(); So EventSender is unable learn context menu items. Ryosuke do you know if this is a limitation on windows of eventsender?

Andrey Petrov

Comment 58 2011-06-02 22:26:54 PDT

anyway, what we can do on windows is to abort the test and consider it passed if we learn there is nothing coming from contextClick(). Since we abort the test technically it is a lie and it did not 'pass' as such. Is it a better option than to skip the test on windows?

Ryosuke Niwa

Comment 59 2011-06-02 22:35:46 PDT

(In reply to comment #58) > anyway, what we can do on windows is to abort the test and consider it passed if we learn there is nothing coming from contextClick(). Since we abort the test technically it is a lie and it did not 'pass' as such. > > Is it a better option than to skip the test on windows? We normally add failing test expectation and file a bug. +jberlin & +aroben although aroben seems to be on vacation now.

Jessie Berlin

Comment 60 2011-06-03 07:49:27 PDT

(In reply to comment #59) > (In reply to comment #58) > > anyway, what we can do on windows is to abort the test and consider it passed if we learn there is nothing coming from contextClick(). Since we abort the test technically it is a lie and it did not 'pass' as such. > > > > Is it a better option than to skip the test on windows? > > We normally add failing test expectation and file a bug. +jberlin & +aroben although aroben seems to be on vacation now. Adam is on vacation for the next couple of weeks. Technically I am on vacation today and Monday, but I can’t resist the urge to check my email ;) In a case like this, where the cause of the failure is that we do not have a DRT / WKTR implementation, we usually add the test to the Skipped list. You should add it under the section for contextClick: # Need to dump context menu items on eventSender.contextClick(true). # https://bugs.webkit.org/show_bug.cgi?id=39104 editing/spelling/context-menu-suggestions.html I did this for mac-wk2 yesterday because WKTR does not have an implementation of eventSender.contextClick.

Note You need to log in before you can comment on or make changes to this bug.