199927 - nsPluginInstanceOwner::Init causes content area to lose focus

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Brian Ryner (not reading)]

Found this while testing scenarios for bug 170811 (this bug turns out to be
totally separate):

Click "Prisoners of War and MIA" link from http://cnn.com/.  Note that keyboard
focus is not in the newly loaded page as it should be.  It's sort of a race
condition, so it may not happen depending on network and CPU speed.

The cause seems to be the code in nsPluginInstanceOwner::Init() which calls
nsDocumentViewer::Show().  This can happen prior to the paint suppression timer
firing.  In this case, however, we don't suppress the focus controller or call
CheckForFocus to transfer focus to the new document, so focus ends up on the
toplevel window (on win32, it may well disappear entirely on mac or linux).  I
think this should be consolidated into an API on the PresShell which forces
painting to be unsuppressed early, if the plugin code really needs that.

Comment 1

[Peter Lubczynski]

Attachment: patch to use nsIPresShell->UnsuppressPainting

Here's a patch to use the pres shell's method, assuming it does the right
thing. Still need to do some testing.... Also with the patch, in seeing the
extra checks for content/reciever in register/removing the DOM listeners, I
think it's safe to remove those.

Comment 2

[Jesse Ruderman]

Do you know what plugin
http://www.cnn.com/SPECIALS/2003/iraq/forces/pow.mia/index.html uses?

See also bug 183103, "java applets steal focus from page".

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 118946 [details] [diff] [review]
patch to use nsIPresShell->UnsuppressPainting

>Index: nsObjectFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsObjectFrame.cpp,v
>retrieving revision 1.384
>diff -u -9 -r1.384 nsObjectFrame.cpp
>--- nsObjectFrame.cpp	24 Mar 2003 04:14:51 -0000	1.384
>+++ nsObjectFrame.cpp	31 Mar 2003 17:01:11 -0000
>@@ -3738,91 +3738,65 @@
>   // stop the timer explicitly to reduce reference count.
>   CancelTimer();
> 
>   // unregister context menu listener
>   if (mCXMenuListener) {
>     mCXMenuListener->Destroy(mOwner);    
>     NS_RELEASE(mCXMenuListener);
>   }
> 
>-  // Unregister focus event listener
>   if (content) {
>     nsCOMPtr<nsIDOMEventReceiver> receiver(do_QueryInterface(content));
>     if (receiver) {

Yeah, you can go ahead and remove the |if (content)| here too, since the
do_QueryInterface covers it.  Please reindent these sections when you remove
|if| blocks, too.

>@@ -3972,91 +3946,68 @@
> NS_IMETHODIMP nsPluginInstanceOwner::Init(nsIPresContext* aPresContext, nsObjectFrame *aFrame)
> {
>   //do not addref to avoid circular refs. MMP
>   mContext = aPresContext;
>   mOwner = aFrame;
>   
>   nsCOMPtr<nsIContent> content;
>   mOwner->GetContent(getter_AddRefs(content));
>   
>-  // This is way of ensure the previous document is gone. Important when reloading either 
>-  // the page or refreshing plugins. In the case of an OBJECT frame,
>-  // we want to flush out the prevous content viewer which will cause the previous document
>-  // and plugins to be cleaned up. Then we can create our new plugin without the old instance
>-  // hanging around.
>-
>-  nsCOMPtr<nsISupports> container;
>-  aPresContext->GetContainer(getter_AddRefs(container));
>-  if (container) {
>-    nsCOMPtr<nsIDocShell> cvc(do_QueryInterface(container));
>-    if (cvc) {
>-      nsCOMPtr<nsIContentViewer> cv;
>-      cvc->GetContentViewer(getter_AddRefs(cv));
>-      if (cv)
>-        cv->Show();
>-    }
>-  }
>+  // Some plugins require a specific sequence of shutdown and startup when
>+  // a page is reloaded. Shutdown happens usually when the last instance
>+  // is destroyed. Here we make sure the plugin instance in the old
>+  // document is destroyed before we try to create the new one.
>+  nsCOMPtr<nsIPresShell> shell (do_QueryInterface(mContext));
>+  if (shell)
>+    shell->UnsuppressPainting();

Um.  PresContext doesn't QI to nsIPresShell.  Use GetShell().

Comment 4

[Brian Ryner (not reading)]

Jesse:

<OBJECT ID="IERPCtl" WIDTH=0 HEIGHT=0
CLASSID="CLSID:FDC7A535-4070-4B92-A0EA-D9994BCC0DC5"></OBJECT>

is inserted into the document via document.write().

Comment 5

[Brian Ryner (not reading)]

Attachment: revised patch

How about this one? I was able to clean up the event listener code a lot.

Comment 6

[Peter Lubczynski]

Comment on attachment 119021 [details] [diff] [review]
revised patch

r=peterl, much better :)

Comment 7

[Brian Ryner (not reading)]

checked in.

Comment 8

[Brian Ryner (not reading)]

Ok, so I decided I don't like unsuppressing painting this early.  We're
effectively defeating paint suppression on any page with a plugin.  I think it
might be better to revert to the old approach, but suppress the focus controller
around the call to cv->Show().

Comment 9

[Brian Ryner (not reading)]

Attachment: follow-up patch

Don't unsuppress painting; instead, explicitly suppress focus and show the new
content viewer.

Comment 10

[Brian Ryner (not reading)]

Attachment: minor tweak to last patch

Comment 11

[Brian Ryner (not reading)]

checked in.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381