Bug 31287 – plugins/document-open.html failed on Leopard Release bot

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r50717%20(7067)/plugins/document-open-pretty-diff.html

I think it's just flakey.
--- layout-test-results/plugins/document-open-expected.txt	2009-11-09 21:31:25.000000000 -0800
+++ layout-test-results/plugins/document-open-actual.txt	2009-11-09 21:31:25.000000000 -0800
@@ -1 +1,2 @@
-CONSOLE MESSAGE: line 0: PLUGIN: DOCUMENT OPEN SUCCESS
+
+This tests that document.open invoked by a plugin via NPN_Invoke without a javascript context succeeds.

CCing author and reviewer.

[Adam Barth]

Maybe there is a race?

[Ananta Iyengar]

I will take a look at this. Having a hard time reproducing it.

[Eric Seidel (no email)]

So far I've only seen it fail once on the bots, so this bug is far from urgent.   I've filed this so that we can track if it fails repeatedly.  I suspect that it's possible for the bots to be slow enough to cause the timeout to fire before the plugin does the document.open.

[Adam Barth]

Can we have the plugin navigate the document to a JavaScript URL that ends the test instead of using a timeout?

[Ananta Iyengar]

Created attachment 42900 [details]
Initial patch to fix the flaky document-open.html plugin layout test.

[Ananta Iyengar]

Created attachment 42901 [details]
Updated patch with incorrect indentations fixed.

[Adam Barth]

Comment on attachment 42901 [details]
Updated patch with incorrect indentations fixed.

Note that the first ChangeLog is in the wrong folder (it's in the root instead of the WebCore folder), so this patch cannot be committed as-is.

I'll leave reviewing the actual code to plug-in experts.

[Ananta Iyengar]

Created attachment 42903 [details]
Updated patch with the WebCore Changelog now in the right place

[Eric Seidel (no email)]

Comment on attachment 42903 [details]
Updated patch with the WebCore Changelog now in the right place

Braces on new lines:
 38 static void pluginLogWithWindowObject(NPObject* windowObject, NPP instance, const char* message) {

check-webkit-style might help find some of these.  It's just webkit's version of cpp_lint.py

[Eric Seidel (no email)]

I'm not sure who the plugin experts are these days, but I CC'd kevin who I think worked on this code.

[Ananta Iyengar]

Created attachment 42909 [details]
Updated patch addressing review comments from eseidel

[Ananta Iyengar]

Created attachment 42968 [details]
Updated patch with more style fixes to make the code consistent with Webkit style

[Eric Seidel (no email)]

I wonder if m_client shouldn't manage its own lifetime.  Seems a little strange for stop() to clear m_client.  But I really know next-to-nothing about this area of code.

[Ananta Iyengar]

The stream is created by PluginView when it receives a response. The PluginView instance pointer is passed in to the create call which is saved as m_client.

When the plugin view is going away it calls stop on existing plugin stream instances and releases them. 

I think setting m_client to NULL in stop is safe. We could look into getting m_client to manage its own lifetime in a subsequent patch.

[Ananta Iyengar]

Created attachment 42977 [details]
Fixed line endings in the layout tests.

[Adam Barth]

Comment on attachment 42977 [details]
Fixed line endings in the layout tests.

Ok

[WebKit Commit Bot]

Comment on attachment 42977 [details]
Fixed line endings in the layout tests.

Rejecting patch 42977 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
ebCore/plugins/PluginStream.cpp
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469

[Ananta Iyengar]

Created attachment 43131 [details]
Updated patch addressing review comments from eseidel

[Ananta Iyengar]

Created attachment 43132 [details]
The reviewed by comment in the webcore changelog was moved incorrectly below the comment. New patch fixes that.

[WebKit Commit Bot]

Comment on attachment 43132 [details]
The reviewed by comment in the webcore changelog was moved incorrectly below the comment. New patch fixes that.

Rejecting patch 43132 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
Tree/TestNetscapePlugIn.subproj/PluginObject.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/LayoutTests/ChangeLog

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

[Ananta Iyengar]

Created attachment 43134 [details]
Updated patch with tabs removed hopefully.

[Adam Barth]

Comment on attachment 43134 [details]
Updated patch with tabs removed hopefully.

This is why we need a try server.  :)

[WebKit Commit Bot]

Comment on attachment 43134 [details]
Updated patch with tabs removed hopefully.

Clearing flags on attachment: 43134

Committed r50929: <http://trac.webkit.org/changeset/50929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.