Bug 35350 – DumpRenderTree should allow tests with modal dialogs

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

[James Robinson]

We probably have bugs regarding modal dialogs that are impossible to test with DRT currently.  See https://bugs.webkit.org/show_bug.cgi?id=33962 for an example of something that's not possible to test currently.  DumpRenderTree should allow writing tests for things like these.

For window.showModalDialog() tests, there isn't much work to do as long as the opened window window.close()s itself.  To test window.confirm() or window.alert() we'll need some API exposed to script that ensures that the dialog self-closes.  For example, the test in 33962 using window.alert() would have to look something like this:

// test setup code
if (layoutTestController)
  layoutTestController.closeNextModalDialogAfter(1); // schedule next modal dialog to close itself after running for 1ms, to ensure that the event queue pumps
window.alert("When running manually hit OK to close this");
// make assertions about what happened while the modal dialog is up

[Prasad Tammana]

Created attachment 57183 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

[Prasad Tammana]

Created attachment 57372 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

[Dmitry Titov]

I think we need someone more familiar with Cocoa to take a look. Feel free to flip r? up.

I see you disabling the tests that fail anyways, considering this is the first patch and  you are on a path to implement the showModalDialog for Win and Chromium as well (at least), it looks as right thing to do and prevents proliferation of platform-specific expectations.

My couple of tiny nits:

LayoutTests/ChangeLog:15
 +          that the method is undefined.  Added mac specific expected output files for all those tests.
That last sentence is probably not entirely correct now. Maybe something like this:  "Modified the tests accordingly and will do more followup (see bug 12345)"

LayoutTests/ChangeLog:67
 +          if deemed important enough.
To add a test for existing functionality is always super-important :-) I think it's best to remove "if deemed important enough"...

[Prasad Tammana]

Created attachment 57603 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac

Addressed all of Dmitry's comments in the latest upload of the patch.  Please see his comments in the bug.

[Darin Adler]

Comment on attachment 57603 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac

> -    [m_webView performSelector:@selector(_closeWindow) withObject:nil afterDelay:0.0];
> +    [m_webView performSelector:@selector(_closeWindow) withObject:nil afterDelay:0.0
> +        inModes:[NSArray arrayWithObjects:NSDefaultRunLoopMode, NSModalPanelRunLoopMode,
> +                 NSConnectionReplyMode, NSEventTrackingRunLoopMode, nil]];

This change is not needed to make showModalDialog work in DumpRenderTree. Making this change could affect the behavior of applications on Mac and so the decision of whether to change it should not be driven by the needs of our test tool alone.

The rest of the patch seems OK.

[Darin Adler]

The override of NSApplication should be done in DumpRenderTree instead of changing WebKit to make DumpRenderTree work. If we later decide to change NSApplication then we can remove the code from DumpRenderTree.

[Prasad Tammana]

(In reply to comment #6)
> The override of NSApplication should be done in DumpRenderTree instead of changing WebKit to make DumpRenderTree work. If we later decide to change NSApplication then we can remove the code from DumpRenderTree.

I tried this but I'm seeing random crashes in runModalCleanup after calling abortModal.  I need to call abortModal to get out of the runModalForWindow call.  I looked at with a "local Mac expert" but we couldn't figure why this would happen.  My broad conclusion is that NSApplication pretending to be in normal loop mode when its not, is interacting badly with the cleanup code from runModalForWindow.  Unscientific, but that's as far as I could go in terms of analyzing this random crash.

Please see below for more comments on your response.

(In reply to comment #5)
> (From update of attachment 57603 [details])
> > -    [m_webView performSelector:@selector(_closeWindow) withObject:nil afterDelay:0.0];
> > +    [m_webView performSelector:@selector(_closeWindow) withObject:nil afterDelay:0.0
> > +        inModes:[NSArray arrayWithObjects:NSDefaultRunLoopMode, NSModalPanelRunLoopMode,
> > +                 NSConnectionReplyMode, NSEventTrackingRunLoopMode, nil]];
> 
> This change is not needed to make showModalDialog work in DumpRenderTree. Making this change could affect the behavior of applications on Mac and so the decision of whether to change it should not be driven by the needs of our test tool alone.

I tried several things but couldn't find any other way to close a modal dialog from script.  As you mentioned above, this affects the behavior of apps on Mac which is why I factored this one change into a separate patch to get a focused review on just this - https://bugs.webkit.org/show_bug.cgi?id=40036.  You reviewed that and gave me feedback to which I responded.  I haven't seen your response to my last set of comments.  Not sure if you had a chance to look at that.  We can continue discussion on that in that bug, so we don't duplicate that here.  I gave detailed comments on why showModalDialog needs this change, to support being able to close from script.

Its a whole another thing if you think we shouldn't be able to close showModalDialog from script, in which case, I'll can to go with my plan B to make this work and upload a new patch.  My plan B is to add abort method to LayoutTestController and invoke that from my test script.  That was my initial approach and I implemented that but switched to what I have now following feedback from a team mate who thought we should fix this in WebKit instead of working around it in LayoutTestController.

> The rest of the patch seems OK.

Thanks for reviewing this somewhat lengthy patch.  Happy to see that I'm getting close to being able to land this.

[Darin Adler]

(In reply to comment #7)
> My broad conclusion is that NSApplication pretending to be in normal loop mode when its not, is interacting badly with the cleanup code from runModalForWindow.

That seems probably wrong to me.

> Its a whole another thing if you think we shouldn't be able to close showModalDialog from script

Does closing the dialog work in Internet Explorer? Since showModalDialog originates there, I’d like to match its behavior if possible.

> My plan B is to add abort method to LayoutTestController and invoke that from my test script.

Our decisions about the web platform should not be based on the needs of DumpRenderTree, so whether we can close a showModalDialog without using the test controller is something we should decide based on what we want on the web, not the needs of our test.

[Prasad Tammana]

(In reply to comment #8)
> (In reply to comment #7)
> > My broad conclusion is that NSApplication pretending to be in normal loop mode when its not, is interacting badly with the cleanup code from runModalForWindow.
> 
> That seems probably wrong to me.

Possible...
 
> > Its a whole another thing if you think we shouldn't be able to close showModalDialog from script
> 
> Does closing the dialog work in Internet Explorer? Since showModalDialog originates there, I’d like to match its behavior if possible.

I just tested this on IE - closing showModalDialog from script works on IE.  Interestingly closing showModalDialog from script also works on Safari.  Seems like Safari does not go through WebChromeClient::closeWindowSoon() when closing modal dialogs.  So its just applications that rely on WebChromeClient that are not able to close modal dialogs from script.

> > My plan B is to add abort method to LayoutTestController and invoke that from my test script.
> 
> Our decisions about the web platform should not be based on the needs of DumpRenderTree, so whether we can close a showModalDialog without using the test controller is something we should decide based on what we want on the web, not the needs of our test.

Agreed.  Its just that this seemed like a bug in WebKit which is why I tried to "fix" it.  If we decide that the current behavior (i.e preventing closing showModalDialog window from script), then I'll update my patch to add abortModal to LayoutTestController (which is test code) and leave the WebKit platform code untouched.

[Prasad Tammana]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> 
> > > Its a whole another thing if you think we shouldn't be able to close showModalDialog from script
> > 
> > Does closing the dialog work in Internet Explorer? Since showModalDialog originates there, I’d like to match its behavior if possible.
> 
> I just tested this on IE - closing showModalDialog from script works on IE.  Interestingly closing showModalDialog from script also works on Safari.  Seems like Safari does not go through WebChromeClient::closeWindowSoon() when closing modal dialogs.  So its just applications that rely on WebChromeClient that are not able to close modal dialogs from script.

I just debugged Safari to confirm what I said above regarding Safari not going through WebChromeClient::closeWindowSoon().  Turns out I'm wrong and Safari does use WebChromeClient and WebChromeClient::closeWindowSoon() does get called.  I looked in the debugger to see the current run loop mode at that point and its kCFRunLoopDefaultMode, and runModalForWindow() is on the stack.  Seems like Safari is overriding NSApplication to make performSelector succeed, just as you suggested to me.

As I said, I was getting a random crash after calling abortModal() when I tried that.  You can see exactly what I'm trying to do in my changes to WebKitTools/DumpRenderTree/mac/UIDelegate.mm in the patch.  Would you know if Safari does something different in its UIDelegate to break out of the runModalForWindow() call?

I guess I have more digging to do to see why I'd get this random crash and not Safari...  or go with what was working for me and add abortModal function to LayoutTestController and make it available for script.  Appreciate any thoughts.  Thanks.

[Darin Adler]

Yes, I checked and Safari does call abortModal.

[Prasad Tammana]

Created attachment 58829 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

Darin - I updated a new patch with my alternate solution which is to add abortModal method to LayoutTestController and expose it to script.  And undid my change to the performSelector method.

So, in summary I have tried out three approaches I'm listing with what in my opinion are the pros and cons.

1) My earlier patch while does the inModes version of performSelector but enabling it for only two modes - normal loop and panel mode.
    Pros:
      a) It works.
      b) Impact is narrow and well defined.
      c) You'll now be able to close modal dialogs from script without overriding NSApplication in your app.  I consider that bug fix.
    Cons:
      a) Its technically a backward incompatible change.  Apps meeting all the conditions below will break:
          - Its a non-safari app AND
          - Its built on top of webkit AND
          - It does not override NSApplication or does some other trick to make closing from script work in modal loop AND
          - It implements and uses showModalDialog AND
          - It has code trying to close the modal dialog from script (that would basically be dead code currently)
          If all the above conditions are met, then the modal dialog will now be closing where it didn't before.

2) My new patch which adds abortModal method to LayoutTestController and makes it available to script.
    Pros:
      a) It works.
      b) No change to non-test code.
    Cons:
      a) Test implementation slightly different from Safari.  Probably doesn't mean much since its already different anyway.

3) Your suggestion about overriding NSApplication method to pretend to be in normal mode when actually in panel mode.
     Pros:
       a) Test matches Safari implementation.
     Cons:
       a) Doesn't work for me.  I tried this again to be sure and I'm getting consistent crashes with the following stack.  Not sure what trick I'm missing in terms of setting this up right.
            objc[46326]: FREED(id): message isSheet sent to freed object=0x10c40b0
            #0 0x976dabfa in _objc_error
            #1 0x976dac30 in __objc_error
            #2 0x976d9637 in _freedHandler
            #3 0x902a5c7e in runModalCleanup
            #4 0x903c9b04 in -[NSApplication runModalForWindow:]
            #5 0x00031c2e in -[UIDelegate webViewRunModal:] at UIDelegate.mm:80
            #6 0x00bf0d5e in CallDelegate at WebDelegateImplementationCaching.mm:89
            #7 0x00bf0dc8 in CallUIDelegate at WebDelegateImplementationCaching.mm:420
            #8 0x00be3d09 in WebChromeClient::runModal at WebChromeClient.mm:267
       b) Overriding NSApplication to make this work in theory impacts all the tests which could be risky.

I'm fine going with either 1) or 2).  Its obviously going to be your call if you think the small risk of backward incompatibility of 1) is acceptable or not - based on your response
so far, I'm guessing its not.  2) could be the way to go.  If it has to be 3) for some reason, I'll need help moving forward with that.

Let me know your thoughts.  Thanks.

[Darin Adler]

Please do (2) for now. We can try something later that has affects outside DRT.

[Darin Adler]

Please do (2) for now. We can try something later that has effects outside DRT.

[Prasad Tammana]

(In reply to comment #14)
> Please do (2) for now. We can try something later that has effects outside DRT.

Sounds good to me.  The newest patch that I uploaded today implements 2).  Please take a look and approve it if looks ok.  Once I get the approval, I'll work on getting it committed.  Thanks.

[Prasad Tammana]

(In reply to comment #15)
> (In reply to comment #14)
> > Please do (2) for now. We can try something later that has effects outside DRT.
> 
> Sounds good to me.  The newest patch that I uploaded today implements 2).  Please take a look and approve it if looks ok.  Once I get the approval, I'll work on getting it committed.  Thanks.

Darin - I set the commit-queue:? flag on the last patch I uploaded which implements your recommendation i.e 2).  The only differences in the new patch from the previous one that you've reviewed would be:

1) I reverted the change to performSelector in WebChromeClient.mm
2) I added abortModal method to LayoutTestController and invoked it from the two new tests that I added.  Files affect by this would be:
    a) DumpRenderTree/mac/LayoutTestControllerMac.mm
    b) DumpRenderTree/LayoutTestController.cpp
    c)  DumpRenderTree/LayoutTestController.h
    d) LayoutTests/fast/events/resources/modal-dialog.html
    e) LayoutTests/fast/harness/resources/modal-dialog.html

If the changes look ok to you, please set commit-queue:+ on the patch.  Thanks.

[Darin Adler]

Comment on attachment 58829 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

> +#if PLATFORM(MAC)

While the need for this may be specific to the Mac, I think the concept can exist cross platform. We could use a less specific name for the function, but I suggest we have an empty function on other platforms rather than not function at all. In the future it could make it easier to write tests. We don’t want a long term strategy that adds something that’s only for one platform.

I’m going to say it’s OK to land this test as-is, but I don’t completely agree with every aspect of the approach here. Disabling tests that are expected to fail is not the best way to deal with them, and there’s a lot of test disabling here. I also don’t like patches with promises for the future in them.

I don’t understand what the 10.2.2 tests have to do with showModalDialog.

[Prasad Tammana]

(In reply to comment #17)
> (From update of attachment 58829 [details])
> > +#if PLATFORM(MAC)
> 
> While the need for this may be specific to the Mac, I think the concept can exist cross platform. We could use a less specific name for the function, but I suggest we have an empty function on other platforms rather than not function at all. In the future it could make it easier to write tests. We don’t want a long term strategy that adds something that’s only for one platform.

That makes sense.  I didn't realize that I'm making the test Mac specific by doing this.  I'll do another patch to add an empty function and make this platform independent once this patch goes through - https://bugs.webkit.org/show_bug.cgi?id=40864.  And later when I implement this for other platforms, I'll rename/re-factor as appropriate.

> I’m going to say it’s OK to land this test as-is, but I don’t completely agree with every aspect of the approach here. Disabling tests that are expected to fail is not the best way to deal with them, and there’s a lot of test disabling here. I also don’t like patches with promises for the future in them.

The other way to handle that I considered and initially implemented was to create a platform specific expected output for MAC.  That would've obviated the need to skip tests but comes with two downsides:
1) The patch was 350k making it huge.
2) It'd be cumbersome and error prone to keep the mac specific expected output files in sync with the rest as some of these files are huge.

> I don’t understand what the 10.2.2 tests have to do with showModalDialog.

The theme of these tests is - "scope chain must contain same objects in the same order as the calling context".  The test concatenates all the properties on the window object in the current context and in eval context and compares the strings.  showModalDialog being undefined seems to cause it to end up at different places in the sort order in different contexts.  And so all these tests fail.  Implementing showModalDialog method caused the tests to start passing.  One could say that these were (effectively) disabled on all platforms and now enabled on Mac.

I could've fixed these tests to ignore showModalDialog in creating this concatenated string and have them pass on all platforms.  As I understand, these are imported from somewhere and not supposed to be modified.

As an aside, these are terribly written tests.  Try this:
LayoutTests/fast/js/sputnik/Conformance/10_Execution_Contexts/10.2_Entering_An_Execution_Context/10.2.2_Eval_Code/
tkdiff S10.2.2_A1.1_T3.html S10.2.2_A1.1_T7.html

You'll see a couple lines of differences between the two files each of which is about 100+ lines and there are couple dozen of these files.  Can't imagine there not being a better way to do this.

Thanks for the review.

[Darin Adler]

(In reply to comment #18)
> As I understand, these are imported from somewhere and not supposed to be modified.

I believe these were written by the V8 team at Google.

[WebKit Commit Bot]

Comment on attachment 58829 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

Rejecting patch 58829 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1
Last 500 characters of output:
omium/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/Skipped
Hunk #1 FAILED at 5937.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 5410.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 FAILED at 893.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej

Full output: http://webkit-commit-queue.appspot.com/results/3282358

[WebKit Review Bot]

http://trac.webkit.org/changeset/61551 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/61550
http://trac.webkit.org/changeset/61551

[Adam Roben (:aroben)]

Comment on attachment 58829 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

Three tests are now failing on Windows because of this change:

fast/dom/Window/window-function-frame-getter-precedence.html
fast/dom/Window/window-function-name-getter-precedence.html
fast/harness/show-modal-dialog.html

See <http://build.webkit.org/results/Windows%20Debug%20(Tests)/r61558%20(15282)/results.html>.

[Andrew Wilson]

(In reply to comment #22)
> (From update of attachment 58829 [details])
> Three tests are now failing on Windows because of this change:
For the record, this patch was landed as r61551 and 61555, and was rolled out in r61564 due to the error noted by aroben.

[Prasad Tammana]

Created attachment 59319 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

This patch is the same as previous one reviewed by Darin.  I've addressed a few non-mac platform specific issues identified by the build bots.  Specific changes are:

1) Updated some platform specific expected output files.  In some cases, deleted them where they are no longer different from the default ones.
2) sputnik tests pass on chromium as DRT on chromium has a default implementation for showModalDialog.
3) Fix a logic bug in window-function-name-getter-precedence.html and window-function-frame-getter-precedence.html to skip showModalDialog when its undefined.

[Dmitry Titov]

Comment on attachment 59319 [details]
Patch to add a showModalDialog support for DumpRenderTree on Mac.

r=me

Since Darin already r+'ed the original patch, I've looked over the change in tests. Looks good.
Will land by hand later today if cq won't.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61599 might have broken Qt Linux Release

[Dmitry Titov]

Landed in http://trac.webkit.org/changeset/61599