Bug 31721

Summary: Using jQuery to show/hide IMG elements crashes WebKit
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: ImagesAssignee: 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: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
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

Description Mihnea Ovidenie 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
Comment 1 Mihnea Ovidenie 2009-11-20 06:11:11 PST
Created attachment 43575 [details]
Crash log on Mac
Comment 2 Mihnea Ovidenie 2009-11-20 06:13:26 PST
Created attachment 43576 [details]
Dr watson log on WinXP
Comment 3 Mihnea Ovidenie 2009-11-20 06:14:22 PST
Created attachment 43577 [details]
User dmp on WinXP
Comment 4 Alexey Proskuryakov 2009-11-20 12:56:07 PST
<rdar://problem/7413760>
Comment 5 Mihnea Ovidenie 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
Comment 6 Andrey Petrov 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.
Comment 7 Andrey Petrov 2011-05-18 11:30:52 PDT
Created attachment 93951 [details]
proposed patch

proposed trivial patch for gtk, chromium, qt, win and WinCE
Comment 8 Early Warning System Bot 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
Comment 9 Andrey Petrov 2011-05-18 12:01:13 PDT
Created attachment 93960 [details]
trival patch

trivial patch, fixed a build error;)
Comment 10 Andrey Petrov 2011-05-18 16:15:24 PDT
Created attachment 93997 [details]
proposed patch, take 3

added Mac
Comment 11 Early Warning System Bot 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
Comment 12 Andrey Petrov 2011-05-18 16:26:42 PDT
Created attachment 93998 [details]
trivial patch

again broken Qt and fixed it again
Comment 13 Daniel Bates 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.
Comment 14 Andrey Petrov 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.
Comment 15 Andrey Petrov 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
Comment 16 Ryosuke Niwa 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.
Comment 17 Andrey Petrov 2011-05-18 16:47:07 PDT
Created attachment 94002 [details]
trivial patch

now added Mac for real, elaborated changelog.
Comment 18 Daniel Bates 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.
Comment 19 Daniel Bates 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.
Comment 20 Andrey Petrov 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
Comment 21 Andrey Petrov 2011-05-19 21:33:44 PDT
Created attachment 94169 [details]
new patch

new patch.
this time with a brand new layout test.
Comment 22 WebKit Review Bot 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
Comment 23 WebKit Review Bot 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
Comment 24 Andrey Petrov 2011-05-19 22:15:06 PDT
Created attachment 94174 [details]
new patch

this time wont break cr-linux hopefully
Comment 25 Andrey Petrov 2011-05-20 05:02:36 PDT
Comment on attachment 94174 [details]
new patch

looks like bots are happy. Asking for review
Comment 26 Adam Barth 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.
Comment 27 Adam Barth 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.
Comment 28 Andrey Petrov 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
Comment 29 Andrey Petrov 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.
Comment 30 Ryosuke Niwa 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?
Comment 31 Andrey Petrov 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
Comment 32 Andrey Petrov 2011-05-20 21:56:57 PDT
Created attachment 94317 [details]
next version 

the new version hopefully addresses Ryosuke's comments.
Comment 33 Andrey Petrov 2011-05-20 22:06:55 PDT
Comment on attachment 94317 [details]
next version 

forgot doctype
Comment 34 Andrey Petrov 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 ;)
Comment 35 Hajime Morrita 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.
Comment 36 Andrey Petrov 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
Comment 37 Mihnea Ovidenie 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
Comment 38 Andrey Petrov 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?
Comment 39 Hajime Morrita 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!
Comment 40 WebKit Commit Bot 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.
Comment 41 WebKit Commit Bot 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
Comment 42 Andrey Petrov 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).
Comment 43 Darin Adler 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.
Comment 44 Andrey Petrov 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?
Comment 45 Daniel Bates 2011-05-26 14:44:55 PDT
CC'ing Darin Adler since Andrey Petrov responded to him in comment 44.
Comment 46 Darin Adler 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.
Comment 47 Andrey Petrov 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.
Comment 48 Darin Adler 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.
Comment 49 Andrey Petrov 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)
Comment 50 Darin Adler 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.
Comment 51 Andrey Petrov 2011-05-27 11:08:40 PDT
Created attachment 95192 [details]
new version

new version also adds isImage() runtime check as suggested
Comment 52 Andrey Petrov 2011-05-27 14:11:33 PDT
Comment on attachment 95192 [details]
new version

bots seem to be happy. asking for review
Comment 53 Hajime Morrita 2011-05-30 20:57:02 PDT
Comment on attachment 95192 [details]
new version

let us try again.
Comment 54 WebKit Commit Bot 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>
Comment 55 WebKit Commit Bot 2011-05-30 22:01:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 57 Andrey Petrov 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?
Comment 58 Andrey Petrov 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?
Comment 59 Ryosuke Niwa 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.
Comment 60 Jessie Berlin 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.