STR: 1. Load http://www.chivian.com/chivian/DungeonsDragons.html 2. Focus the search bar (easiest to test with) 3. reload page (using keyboard shortcut is easiest, ctrl+f5 works) 4. Try typing (search bar will still be focused). If you've waited long enough typing won't work at all, neither will ctrl+f5. If you started typing right away, typing will stop working at some point. Pushlog for regression range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bcd9709de08a&tochange=6712bed154ed I haven't tried reproducing with other sites. I found this site to try and test another bug and it was hard enough to find. Reload is required. First load works fine. Bug happens in latest nightly - Mozilla/5.0 (Windows NT 6.0; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

Forgot to say doesn't happen in Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

I can reproduce this on Windows XP, and also find that it only happens when dom.ipc.plugins.enabled is set to false. I find (basically) the same regression range: firefox-2011-01-27-05-mozilla-central firefox-2011-01-28-05-mozilla-central I also don't see this in FF 3.6.13.

Forgot to mention that the text-entry cursor stays flashing in the Search bar even after it loses focus.

(In reply to comment #2) > I can reproduce this on Windows XP, and also find that it only happens > when dom.ipc.plugins.enabled is set to false. > > I find (basically) the same regression range: > > firefox-2011-01-27-05-mozilla-central > firefox-2011-01-28-05-mozilla-central > > I also don't see this in FF 3.6.13. I've been testing it with clean profiles, so it's been happening with dom.ipc.plugins.enabled set to true. Is that what you meant to say? If you meant false let me know and I'll double check on one of my XP machines.

Is this not the same issue as reported in bug 626016?

> I've been testing it with clean profiles, so it's been happening > with dom.ipc.plugins.enabled set to true. Is that what you meant to > say? Yes. I should have said the opposite of what I said above: This bug doesn't happen with dom.ipc.plugins.enabled is set to false (when it's changed from its default setting). It does happen when this setting is true. Sorry for the confusion :-(

Not a problem. I hit a few false positives in the facebook bug myself ;)

> Is this not the same issue as reported in bug 626016? It really sounds like it. But the regression ranges seem to be different. Lucy, I'll do another test build (a tryserver build) with my patch for bug 618487 reversed. It'll be ready in a few hours. Once it's ready, please test it against both bug 626016 and this bug (bug 629593). (I'm also working on a debug-logging build. That'll take longer.)

(In reply to comment #5) > Is this not the same issue as reported in bug 626016? My guess is that the patch we bisected to in that bug allows quicktime in facebook to behave the way quicktime is doing in this bug. I would imagine fixing this bug fixes the other. (In reply to comment #8) Steven, I did test this bug in the build ehsan gave me with your patch reversed (in case you missed me saying that), and this bug happens long before your patch was checked in, but I'm happy to help test in any way I can if it helps you.

Lucy, the difference between the two regression ranges is so puzzling that I think it's worth doing your tests with Ehsan's build over again. And with my build once it's done. It's hard to believe that Ehsan could have reversed my patch for bug 618487 incorrectly (it's a very simple patch). But the current state of the evidence is so wierd that I think the possibility is worth examining.

> Steven, I did test this bug in the build ehsan gave me with your > patch reversed (in case you missed me saying that) I did miss it. Don't feel obliged to test this bug with Ehsan's build again. But I *would* like you to test bug 626016 with Ehsan's build again. And both bugs with my build when it becomes available (the one I'm now making on the tryservers to reverse my patch for bug 618487).

(In reply to comment #10) > Lucy, the difference between the two regression ranges is so puzzling that I > think it's worth doing your tests with Ehsan's build over again. > > And with my build once it's done. > > It's hard to believe that Ehsan could have reversed my patch for bug 618487 > incorrectly (it's a very simple patch). But the current state of the evidence > is so wierd that I think the possibility is worth examining. I did, several times. This bug has happened for a LONG time. While I don't think Ehsan backed out your patch incorrectly, I also don't think your patch is somehow being injected into builds from January 2010 (btw, you typed 2011, please confirm you meant 2010!) However, even back then this bug wasn't happening on Facebook. Facebook also behaves differently than this page. On this page you can see the Quicktime plugin, on Facebook you never do (not even in DOMInspector). The answer to why does Facebook all of a sudden have this bug is in your patch. The answer to why this bug even happens at all is somewhere else.

(In reply to comment #11) But I *would* like you to test bug 626016 with Ehsan's build > again. I did it each time I tested this bug, to make sure I was not seeing the Facebook problem. Way ahead of you ;)

(From comment #4) > I find (basically) the same regression range: > > firefox-2011-01-27-05-mozilla-central > firefox-2011-01-28-05-mozilla-central (In reply to comment #12) > (btw, you typed 2011, please confirm you meant 2010!) Yes, I did mean 2010. It's been a long week :-( And yes, it's possible that my patch for bug 618487 somehow triggered/uncovered this bug (bug 629593) on the Facebook page. In fact if that's true, I think it's good news! Simply reversing my patch for bug 618487 would bring back bug 618487. So I'm going to assign this bug to myself, and at least try to find out how difficult it would be to fix. It has the advantage of being much easier to reproduce (and I often perform a bug's STR hundreds of times while I'm trying to fix it).

Obviously I agree ;) Though if you still did want me to try anything I will. I just worried it's extra work for yourself that didn't make sense. I'm happy to test out bisected builds for this bug, or anything else that might help. As you said it's quite easy to reproduce, and the music is pleasantly sinister!

(In reply to comment #10) > It's hard to believe that Ehsan could have reversed my patch for bug 618487 > incorrectly (it's a very simple patch). But the current state of the evidence > is so wierd that I think the possibility is worth examining. FWIW, what I did was: hg update -r BETA10_RELBRANCH_CHANGESET_ID wget -O patch http://hg.mozilla.org/mozilla-central/raw-rev/xxxx patch -p1 -R < patch make -f client.mk build

And FWIW, my mysterious build is actually available at <http://people.mozilla.com/~eakhgari/firefox-4.0b10.en-US.win32.zip>.

Thanks, Ehsan. What you did should have worked fine. I did it slightly differently -- I just applied this patch to current code. The results (the tryserver build) should be available fairly soon.

Here (finally) is the tryserver build made with my "patch" from comment #18: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-ed7b233805b5/try-w32/firefox-4.0b11pre.en-US.win32.zip

I still see this bug in that build. Facebook works fine. Btw, I do want to apologize for being impatient with you. I'd just done quite a lot of work testing over a couple days, so I was obviously put off when you were reluctant to use facebook. But if you hadn't been, and if I hadn't been impatient, then I wouldn't have found this bug! Very funny how things turn out sometimes.

Working on complex bugs like this enforces patience. Otherwise you go completely nuts :-)

If I flip dom.ipc.plugins.enabled to true in branch then the bug shows itself. I think we need to find a new regression window by testing with that pref flipped (if you're not already ahead of me ;) )

> If I flip dom.ipc.plugins.enabled to true in branch then the bug > shows itself. I'm not surprised. I forgot to mention earlier that the trunk regression range we both found contains the following patch: http://hg.mozilla.org/mozilla-central/rev/f54bb3222492 Benjamin Smedberg — Bug 531142 - Turn on multi-process plugins by default And if we explicitly set dom.ipc.plugins.enabled to true, the problem occurs (on the trunk) earlier than this range. So the patch for bug 531142 triggered this bug, but it has probably existed all along in OOPP code (on Windows). (By the way, I've been having a lot of trouble writing and building a debug patch that's usable on Windows. But I should have one by tomorrow.)

Sounds good! I'm using harthur's regression range finder - http://harthur.github.com/mozregression/ which makes things a LOT less stressful than when I did the Facebook window ;) So I'm on that now. Hopefully it'll give us a useful clue.

Oy. Ok so tested by checking for, and flipping dom.ipc.plugins.enabled to true. There are problems from the electrolysis merge, but at first minefield crashes hard just loading the test page - get a windows crash message, not one from minefield - so I obviously couldn't test for this bug. That starts here http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44c392db6672&tochange=96e8d529b2d3 Then the crashing stops a couple days later, and then we can see this problem happening. One of bsmedberg's checkins makes a reference to fixing crashes with plugins, but nothing jumps out and screams "fixing quicktime crash" Crashing stops here - http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-12-16&enddate=2009-12-17 and this bug is immediately reproducible. No idea what happens in electrolysis builds from before the merge, that's beyond my resources.

Not blocking on this per blocker triage meeting.

(Following up bug 626016 comment #124) > The error is "Unknown x86 instruction byte 0xb8". The assembler code for SetFocus() (in 32-bit mode) is wierd. It doesn't seem to use the "normal" stack frame. (gdb) x/16xb 0x7e42b112 0x7e42b112 <USER32!SetFocus>: 0xb8 0x03 0x12 0x00 0x00 0xba 0x00 0x03 0x7e42b11a <USER32!SetFocus+8>: 0xfe 0x7f 0xff 0x12 0xc2 0x04 0x00 0x90 (gdb) disassemble 0x7e42b112 Dump of assembler code for function USER32!SetFocus: 0x7e42b112 <USER32!SetFocus+0>: mov $0x1203,%eax 0x7e42b117 <USER32!SetFocus+5>: mov $0x7ffe0300,%edx 0x7e42b11c <USER32!SetFocus+10>: call *(%edx) 0x7e42b11e <USER32!SetFocus+12>: ret $0x4 0x7e42b121 <USER32!SetFocus+15>: nop 0x7e42b122 <USER32!SetFocus+16>: nop 0x7e42b123 <USER32!SetFocus+17>: nop 0x7e42b124 <USER32!SetFocus+18>: nop 0x7e42b125 <USER32!SetFocus+19>: nop End of assembler dump. Here, by contrast, is the assembler for SetWindowLongA(): (gdb) x/16xb 0x7e42c29d 0x7e42c29d <USER32!SetWindowLongA>: 0x8b 0xff 0x55 0x8b 0xec 0x6a 0x01 0xff 0x7e42c2a5 <USER32!SetWindowLongA+8>: 0x75 0x10 0xff 0x75 0x0c 0xff 0x75 0x08 (gdb) disassemble 0x7e42c29d Dump of assembler code for function USER32!SetWindowLongA: 0x7e42c29d <USER32!SetWindowLongA+0>: mov %edi,%edi 0x7e42c29f <USER32!SetWindowLongA+2>: push %ebp 0x7e42c2a0 <USER32!SetWindowLongA+3>: mov %esp,%ebp 0x7e42c2a2 <USER32!SetWindowLongA+5>: push $0x1 0x7e42c2a4 <USER32!SetWindowLongA+7>: pushl 0x10(%ebp) 0x7e42c2a7 <USER32!SetWindowLongA+10>: pushl 0xc(%ebp) 0x7e42c2aa <USER32!SetWindowLongA+13>: pushl 0x8(%ebp) 0x7e42c2ad <USER32!SetWindowLongA+16>: call 0x7e42c256 <USER32!DefWindowProcA+216> 0x7e42c2b2 <USER32!SetWindowLongA+21>: pop %ebp 0x7e42c2b3 <USER32!SetWindowLongA+22>: ret $0xc 0x7e42c2b6 <USER32!SetWindowLongA+25>: nop 0x7e42c2b7 <USER32!SetWindowLongA+26>: nop 0x7e42c2b8 <USER32!SetWindowLongA+27>: nop 0x7e42c2b9 <USER32!SetWindowLongA+28>: nop 0x7e42c2ba <USER32!SetWindowLongA+29>: nop End of assembler dump.

I forgot to mention that I've been testing on Windows XP SP3 (fully patched).

_NtUserSetFocus@4: 75A61B99 B8 51 10 00 00 mov eax,1051h 75A61B9E B9 03 00 00 00 mov ecx,3 75A61BA3 8D 54 24 04 lea edx,[esp+4] 75A61BA7 64 FF 15 C0 00 00 00 call dword ptr fs:[0C0h] 75A61BAE 83 C4 04 add esp,4 75A61BB1 C2 04 00 ret 4 It's a 4 byte value move into the accumulation register. There might be a range of instructions we could catch here, I didn't look deeply at it. Intel has the ugly details: http://www.intel.com/products/processor/manuals/

> It's a 4 byte value move into the accumulation register. There might > be a range of instructions we could catch here, There is -- they all start with a value from '0xb8' through '0xbf', and are each 5 bytes long in 32-bit mode (the lead byte plus the 4-byte immediate value). I checked since I posted my comment :-) I'll try adding this to CreateTrampoline(), and see if that helps.

Finally, after many trials and tribulations, here's a patch for this bug. It hooks SetFocus() in the plugin-container process for the QuickTime plugin on Windows, and eats any call to SetFocus() that occurs during the first call to QuickTime's NPP_SetWindow method. During the first call to QuickTime's NPP_SetWindow() method, it calls SetFocus() on the HWND passed to it by NPP_SetWindow (window->window). (I was incorrect when, in bug 626016 comment #107, I said that QuickTime calls SetFocus() with a NULL parameter -- I got this information from gdb, and must have misunderstood how the stack frame is set up.) This goes against the browser's expectations: As best I can tell, Mozilla never focuses a plugin when it loads it (or reloads it). The plugin should only get keyboard focus if the user clicks on it. For some reason this doesn't cause trouble except when we're doing OOPP. So, to be as conservative as possible, my patch only has effect when we're doing OOPP. I don't know why QuickTime calls SetFocus() on the first call to NPP_SetWindow(), and I don't know if this (or its equivalent) also happens on other platforms than Windows. But as best I can tell this call is unneeded, and eating it doesn't interfere with QuickTime's functionality. Here's a tryserver build made with my patch: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-798eab47bbf9/tryserver-win32/firefox-4.0b12pre.en-US.win32.zip

Comment on attachment 512493 [details] [diff] [review] Fix bsmedberg is the owner of this code. bent could also do the review. One nit, add a check of sUser32SetFocusHookStub in the GetQuirks() if statement before calling AddHook to save a little overhead on SetWindow.

> One nit, add a check of sUser32SetFocusHookStub in the GetQuirks() > if statement before calling AddHook to save a little overhead on > SetWindow. I assume you mean like what's already in PluginInstanceChild::InitPopupMenuHook(). Will do.

Build in comment #32 is WFM!

Comment on attachment 512503 [details] [diff] [review] Fix rev1 (follow Jim's suggestion) I think I prefer the patch felipe made: hooking functions just makes me nervous, and is going to be even more of a pain on win64.

(In reply to comment #36) You (and many others) vastly overestimate the danger of hooking OS functions -- especially in this case, where the function is documented and very unlikely to change. And it'd be quite easy to add support to nsWindowsDllInterceptor for 64-bit mode. Also, think my patch is actually safer than Felipe's, because it's more narrowly targeted. But it's up to you :-) I don't think Felipe's patch is actually unsafe, or likely to cause trouble. It's just somewhat less safe than mine.

I've run out of time for this bug. Someone else will need to take it.

(In reply to comment #37) > (In reply to comment #36) > and very unlikely to change. And it'd be quite easy to add support to > nsWindowsDllInterceptor for 64-bit mode. That's in bug 604302, should be landing fairly soon. bug 606473 covers upgrading existing plugin hooks.

Preventing SetFocus in the NPP_SetWindow call for QuickTime seems a bit hacky. Surely the problem is that Firefox isn't correctly recognising which window has focus? What about plugins apart from QuickTime? There is nothing inherently wrong with calling SetFocus, and it could happen at any time. I'm having a similar problem with may be related. We have a plugin which opens up a new top-level login window and calls SetFocus on it. The first time you go to the page with our plugin, it works fine. But if you reload the page the login window won't let you type into it and it just flashes in the taskbar and the Firefox window appears to be the foreground window. The login window still thinks it has focus (and GetFocus() returns the login window's HWND), but the user can't type. A workaround seems to be to use GetForegroundWindow/GetWindowThreadProcessId/GetGUIThreadInfo instead of GetFocus, but I'm not sure why this is necessary. Surely GetFocus() shouldn't return our window if it doesn't actually have focus? The problem only occurs when using plugin-container.exe - if you switch dom.ipc.plugins.enabled to false everything works fine. I'm not sure if this is a Windows bug, a firefox bug, or just my misunderstanding of how GetFocus works (although in every other situation it works as I would expect - just not with firefox and plugin-container.exe).

This isn't my bug anymore, but I'll say one thing -- it's quite clear to me that it's a bug in the QuickTime plugin for it to call SetFocus() in NPP_SetWindow().

Perhaps I'm missing something, but why exactly is it a bug for QuickTime to call SetFocus() in NPP_SetWindow? You say above that it is 'unnecessary', but that doesn't mean it is the actual cause of this bug - instead it just seems to be a trigger. The underlying cause seems to be that firefox still thinks it has focus when it doesn't. As I have pointed out above, this bug (assuming it is the same bug) occurs in plugins other than quicktime, and in places other than NPP_SetWindow().

In the previous bug we fixed, quicktime was calling SetFocus from SetWindow for _invisible_ plugins, which was quite wrong. In this case, it's fine for it to grab the focus for visible plugins (we already accept this for html <input autofocus> elements, for example). This is a bug in our focus manager which is not realizing that the focus went to the plugin and still considers firefox to be focused.

> In this case, it's fine for it to grab the focus for visible plugins > (we already accept this for html <input autofocus> elements, for > example). It's quite different for a plugin to do this (on its own authority) than for an autofocus element to do it (which is *intended* to grab the focus). And in any case David's plugin does something quite different -- it creates another windows and focuses that. I didn't mean to call *that* a bug. Sorry if I appeared to. David, I think your problem is unrelated to this bug. I suspect you should open a new one.

The bug in our plugin happens on Chrome as well, and I have managed to fix it using the workaround above (using GetGUIThreadInfo() instead of GetFocus() to determine if our window still has focus). Yes, it is kind of the opposite of this bug - in our case it is our plugin's window that thinks it has focus when it doesn't, whereas in this bug it is firefox that is confused about focus. But I suspect it might have the same underlying cause, because in both cases it only happens with firefox's OOPP and exactly the same thing is happening. From my limited research it appears to be a bug in the Windows GetFocus() function that is triggered in situations where there are two processes with the same top-level window. GetFocus() should be returning NULL but is actually returning a window that does not have focus at all.

Another difference between QuickTime and your plugin is that Firefox (in OOPP mode) honors QuickDraw's attempt to grab the focus, but doesn't honor your plugin's attempt to do so. So the situation with QuickDraw seems much less complex than with your plugin, and the Windows bug(s) that you seem to have found may not come into play.

I can reproduce this with http://www.toronto.ca/garbage/single/calendars/friday_2.pdf (and other embedded pdfs) using http://hg.mozilla.org/releases/mozilla-aurora/rev/a5a5c583c381 Only tested on this one so far.

Wrong bug? :-)

I meant to say that the same thing happens using a different plugin. Happy to file a new bug, but since it's the same behaviour thought I'd post here first in case that changes what needs to be fixed.

Can you test with a nightly? A fix landed for focus related issues with background plugins in bug 273456 last week.

This bug doesn't happen with quicktime, but still happens with pdf, latest nightly.

Felipe and I discussed this today in #developers. I believe that fixing bug 601889 will alleviate some of the issues described in this bug (specifically the fact that a plugin taking focus does not cause the main Firefox window to indicate that it has lost focus).

RIP QuickTime for Windows