Closed Bug 1137229 Opened 9 years ago Closed 9 years ago

Keyboard stops working on OS X

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox37 + fixed
firefox38 + fixed
firefox39 + fixed

People

(Reporter: milan, Assigned: smichaud)

References

Details

(Keywords: regression, Whiteboard: [STR in comment #57])

Attachments

(7 files, 6 obsolete files)

Truncated log
45.91 KB, text/plain
Details
Possible fix
962 bytes, patch
masayuki
: review+
lmandel
: approval-mozilla-aurora+
lmandel
: approval-mozilla-beta+
Details | Diff | Splinter Review
Stack of NS_BLUR_CONTENT event on page hide
4.08 KB, text/plain
Details
Stack of NS_BLUR_CONTENT event on transition to display: none
5.86 KB, text/plain
Details
Second fix, what I will reland
6.35 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Second fix, aurora patch
6.28 KB, patch
smichaud
: review+
lmandel
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Second fix, beta patch
6.32 KB, patch
smichaud
: review+
lmandel
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Couple of times now in two days.  Usually starts while in Yahoo Mail, in Dev edition, and keyboard stops working.  After that, even when the Yahoo Mail tab is closed, I now can't type in the URL bar either.  I can, however, cut and paste into the URL bar, but only using the Edit->Paste.  If I Cmd V, I see the Edit menu item highlight, but nothing shows up in the URL bar.

Restarting the browser is the only thing that helps.

Not sure what other info I can collect to help with this, let me know.
Does it help if you focus some other program and then again FF?
Doesn't seem to.  In fact, I had the dev edition open while I was typing this bug in the nightly, and went back a couple of times doing the steps I described above.
Oh, you can reproduce this?

I assume this is a regression?
Keywords: regression, regressionwindow-wanted
I reproduced it twice in two days, but it isn't on command yet, and I'm not quite sure what exact STR leads me to it.  However, yes, it is a new problem for me; my dev edition is currently Feb 23rd, it may very well be he one that started it.
Got it to happen just now in the nightly 2015-03-05.  I know this isn't actionable, but thought I'd record it.  I'll look for an STR - I did have a handful of tabs opened, Flash in some of them, but at the time I ran into this it was just one tab.
Summary: Keyboard stops working on OS X, Dev edition → Keyboard stops working on OS X
Bug 1138466 seems likely to be a duplicate.   

Milan, did this happen in 38 as well as in 39?
status-firefox38: --- → ?
status-firefox39: --- → affected
Flags: needinfo?(milan)
I am seeing this behavior as well on OSX using beta 37.  I notice it when NoScript extension is enabled... but could happen other times as well.

Keyboard stops working altogether in Firefox -- fields, address bar, search, etc -- but works normally outside of firefox.  The only way to fix is close Firefox and restart.
[Tracking Requested - why for this release]:
Apparently happens on FF37 too.
tracking-firefox37: --- → ?
Anyone who can see this, is there anything related in the browser console?
I wonder if this is related to Bug 1110884.
So far the issue has been reported on OSX only, AFAIK.

But then, comment 7 is about FF37.
Bug 1092630 did land for FF37.

Anyone who can see this, is this perhaps related to plugins in a web page?
Tracking 37+ on both reported bugs at this point.
status-firefox37: --- → affected
status-firefox38: ? → affected
tracking-firefox37: ? → +
tracking-firefox38: --- → +
tracking-firefox39: --- → +
(In reply to Olli Pettay [:smaug] from comment #9)
> Anyone who can see this, is there anything related in the browser console?

Milan is going to come by with his machine if and when this issue reoccurs. In that case, what other information can we try and get to help debug the problem?
Flags: needinfo?(bugs)
Yes, I saw this on 38 and 39.  There may have been Flash involved in one of these, and no more than 5-6 tabs total.  Note that the events do get through up to a point - when I tried Command W while I was in this state, I did see the top level menu highlight, but that was it - nothing actually happened.  Same with Command V for paste.
Flags: needinfo?(milan)
Managed to get a "lesser" version of this bug twice in a couple of minutes now in 38.  Went to http://www.theweathernetwork.com/weather/canada/ontario/toronto, scrolled around a bit, picked one of the videos, watched a few seconds, then seeked through to the end, 5, 6 steps, watching a bit each time.

At that point, Command W would not close the window, but the URL bar did take text input, and once it did, the Command W went back to working.

I'll see if this is common enough to do a regression range, if we think this is the same issue.
(In reply to Olli Pettay [:smaug] from comment #11)
> Bug 1092630 did land for FF37.
> 
> Anyone who can see this, is this perhaps related to plugins in a web page?

Yeah, I guess that it's the cause.
https://hg.mozilla.org/releases/mozilla-beta/filelog/21f52f25675a/widget/cocoa/TextInputHandler.mm
Milan, the next time the bug occurs can you try to open a new window (through the
file menu) and see if the keyboard works there? (the reporter in bug 1138466
said so and if it's the case for you too then it's very likely the same bug).

The next time you restart Firefox, please run it with NSPR logging
as suggested by Masayuki-san in bug 1138466 comment 8.
I'm not sure if this actual cause, but looks like this is a bug:

>    1.169 @@ -3302,20 +3285,16 @@ IMEInputHandler::OnDestroyWidget(nsChild
>    1.170       TrueOrFalse(IsIMEComposing())));
>    1.171  
>    1.172    // If we're not focused, the focused IMEInputHandler may have been
>    1.173    // created by another widget/nsChildView.
>    1.174    if (sFocusedIMEHandler && sFocusedIMEHandler != this) {
>    1.175      sFocusedIMEHandler->OnDestroyWidget(aDestroyingWidget);
>    1.176    }
>    1.177  
>    1.178 -  if (!PluginTextInputHandler::OnDestroyWidget(aDestroyingWidget)) {
>    1.179 -    return false;
>    1.180 -  }
>    1.181 -
>    1.182    if (IsIMEComposing()) {
>    1.183      // If our view is in the composition, we should clean up it.
>    1.184      CancelIMEComposition();
>    1.185      OnEndIMEComposition();
>    1.186    }
>    1.187  
>    1.188    mSelectedRange.location = NSNotFound; // Marking dirty
>    1.189    mIMEHasFocus = false;

The bug removes PluginTextInputHandler. At that time, this line is removed. But this called its super class's OnDestroyWidget(). So, TextInputHandlerBase::OnDestroyWidget() is now never called...
updating my prior comment (#7) to note that I am getting this keyboard locking behavior without NoScript installed also.

Opening a new window (through the file menu) does re-enable the keyboard - in the new window only, not the old one.
(Following up comment #18)

I just talked to Masayuki on IRC, and he explained that it was the patch for bug 1092630 that dropped those lines:

https://hg.mozilla.org/releases/mozilla-beta/diff/a6db8f54f5aa/widget/cocoa/TextInputHandler.mm
Whoever sees this bug, please test with builds dated 2014-02-20 and earlier -- especially m-c and Developer Edition nightlies.  Be sure to test with e10s mode off.

Those builds don't contain my patch for bug 1110888.
Attached file Truncated log — 
I've removed everything before I started playing video - let me know if there is missing information.  Line 15 is where we were just before I started pressing Command W to close the tab (didn't work.)

On a side note, opening a new window did not fix this.
People who see this bug:

What version(s) of OS X are you using?
(In reply to comment #15)

> Managed to get a "lesser" version of this bug ...
> At that point, Command W would not close the window ...

This is probably unrelated.

All you need to do to see it happen is click at least once on a (plugin-created) video.  This gives the keyboard focus to the plugin, and from this point it will eat most (all?) cmd-key combinations (like cmd-w).  The problem lasts until the plugin loses the keyboard focus.
(In reply to Steven Michaud from comment #23)
> People who see this bug:
> 
> What version(s) of OS X are you using?

10.9

(In reply to Steven Michaud from comment #24)
> (In reply to comment #15)
> 
> > Managed to get a "lesser" version of this bug ...
> > At that point, Command W would not close the window ...
> 
> This is probably unrelated.
> 
> All you need to do to see it happen is click at least once on a
> (plugin-created) video.  This gives the keyboard focus to the plugin, and
> from this point it will eat most (all?) cmd-key combinations (like cmd-w). 
> The problem lasts until the plugin loses the keyboard focus.

Fair enough.  The File menu item still flashes though.
Attached patch Possible fix — — Splinter Review 
Masayuki, I agree that what you found is a bug.  I also think fixing it will probably also fix this bug.

The patch for bug 1092630 changed the IMEInputHandler class so that it inherited from TextInputHandlerBase rather than PluginTextInputHandler.  Accordingly the patch changed the IMEInputHandler constructor to invoke the TextInputHandlerBase constructor (instead of the PluginTextInputHandler constructor).  It should also have changed the call to the superclass's OnDestroyWidget() (from IMEInputHandler::OnDestroyWidget()), but instead it just dropped the call.

Since then all calls to TextInputHandlerBase::Destroyed() have often returned inaccurate results -- they now always return false!  Notice that these calls are invoked all over the place in TextInputHandler.mm.  This is truly a disastrous bug.  It's amazing the fallout wasn't worse, and that we didn't discover it before now.
Attachment #8575588 - Flags: review?(masayuki)
(Clearing the ni?. I'm not the right person to answer to OSX or plugins specific issues. Besides, others are looking into this already.)
Flags: needinfo?(bugs)
(In reply to Milan Sreckovic [:milan] from comment #22)
> Created attachment 8575424 [details]
> Truncated log
> 
> I've removed everything before I started playing video - let me know if
> there is missing information.  Line 15 is where we were just before I
> started pressing Command W to close the tab (didn't work.)

Thank you for the log. Although, our logging code doesn't collect enough information. I'll improve it. Anyway, this log indicates that TextInputHandler believes that it dispatches key events correctly (perhaps on a destroyed widget).
Comment on attachment 8575588 [details] [diff] [review]
Possible fix

Thanks! Let's try this.
Attachment #8575588 - Flags: review?(masayuki) → review+
Here's the opt tryserver build from comment #27:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-7f8185f65f28/try-macosx64/firefox-39.0a1.en-US.mac.dmg

Whoever sees this bug please test with it (until the patch lands on mozilla-central).
https://hg.mozilla.org/mozilla-central/rev/cd0ee062c7db
Assignee: nobody → smichaud
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8575588 [details] [diff] [review]
Possible fix

This patch is in today's mozilla-central nightly.

Whoever sees this bug, please test with it.
So far, so good for me, not able to reproduce it.
Comment on attachment 8575588 [details] [diff] [review]
Possible fix

This patch fixes a simple/stupid mistake in the patch for bug 1092630.  It's a very bad mistake, which should have had much worse consequences than this bug.  (And we *have* already found at least one crash bug that it caused -- bug 1040939.)

So we should uplift the patch even if it doesn't fix this bug or bug 1138466.

Because the patch fixes a mistake (because it restores code that was mistakenly dropped), I think the risk is low.

Approval Request Comment
[Feature/regressing bug #]: 1092630
[User impact if declined]: Crash bugs and other weird keyboard problems
[Describe test coverage new/current, TreeHerder]: One day baking on m-c
[Risks and why]: Low risk (the patch restores code that was mistakenly dropped)
[String/UUID change made/needed]: None
Attachment #8575588 - Flags: approval-mozilla-beta?
Attachment #8575588 - Flags: approval-mozilla-aurora?
Comment on attachment 8575588 [details] [diff] [review]
Possible fix

Agreed. Let's get this into Beta 5 and try to verify that it does fix the keyboard issue reported in this bug. Beta+ Aurora+
Attachment #8575588 - Flags: approval-mozilla-beta?
Attachment #8575588 - Flags: approval-mozilla-beta+
Attachment #8575588 - Flags: approval-mozilla-aurora?
Attachment #8575588 - Flags: approval-mozilla-aurora+
(Following up comment #36)

> (And we *have* already found at least one crash bug that it caused -- bug 1040939.)

Bug 1140939.
I've tried today to reproduce the original issue and verify this on Firefox 37 Beta 5 (BuildID=20150312193711). I didn't manage to fully block the keyboard on a Macbook Retina 15" with Mac OS X 10.9.5, but I did manage to reproduce the behavior specified in comment 24:
"All you need to do to see it happen is click at least once on a (plugin-created) video.  This gives the keyboard focus to the plugin, and from this point it will eat most (all?) cmd-key combinations (like cmd-w).  The problem lasts until the plugin loses the keyboard focus."

To reproduce the issue I just opened a facebook flash game, then any cmd-key combination no longer worked. Note though that I could still easily reproduce this scenario in Firefox 37 Beta 5 which should have this fix, so it may be that the fix does not work correctly.

Any thoughts on the above Steven?
Flags: needinfo?(smichaud)
(In reply to comment #41)

Like I said in comment #24, this is completely unrelated.

The general question of what key combinations plugins should be able to consume/block (when they have they keyboard focus) is one of the things under discussion at bug 1128771.
Flags: needinfo?(smichaud)
I'm still seeing the full keyboard-locking behavior on Firefox 37 beta 5 - OSX10.10.2.  Did the patch make it into beta 5?
> Did the patch make it into beta 5?

Yes, it did.

Please explain in detail what you mean by the "full keyboard-locking behavior".  Be quite sure that you're not seeing what I described in comment #24 (which like I said is unrelated).

If you can reproduce what you see reliably, please try to find the regression range in mozilla-central nightlies (here http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/).
It was recommended I include my experience with possibly the same issue, or at least one that is very similar. This is my comment from IRC:

Yeah, the keyboard doesn't work, either. The more obvious issue to me is just that the cursor does not affect any state change on elements in the browser chrome or in a Web page. It's just like hovering over a photo; however, the contents of the hover, like a URL, do display at the bottom of the browser as usual. Clicking a link will do nothing for about ten seconds, then the page will load; the same is true for browser chrome, like with tabs. As far as typing, it's fairly non-responsive, though after several seconds the given input does show the text.
Dane, what you describe sounds like a different bug ... though it's really hard to be sure.

Some obvious questions:

What versions of Firefox do you see this on?
What versions of OS X do you see this on?
Does your problem happen with a clean profile?
Do you have reliable steps to reproduce?

Please be sure to include the latest versions of Firefox in your tests -- particularly today's mozilla-central nightly.
(In reply to Steven Michaud from comment #46)
> Dane, what you describe sounds like a different bug ... though it's really
> hard to be sure.

I remember Dane saying on IRC that this was happening after resuming OS X from sleep mode and stuff like that. If that's the case, then I believe that's bug 1142957, which should be fixed in today's Nightly. 

Maybe this issue and bug 1142957 are connected in some way? I guess people still having this issue might want to try with the March 18th Nightly and see if they still have problems.
(In reply to Steven Michaud from comment #44)
> > Did the patch make it into beta 5?
> 
> Yes, it did.
> 
> Please explain in detail what you mean by the "full keyboard-locking
> behavior".  Be quite sure that you're not seeing what I described in comment
> #24 (which like I said is unrelated).
> 
> If you can reproduce what you see reliably, please try to find the
> regression range in mozilla-central nightlies (here
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/).


I'm still seeing the original issue, and not what is described in comment 24.  the keyboard (all keys) stops working in the existing window.  The only way to fix it is to open a new window or close/reopen firefox.  It occurs with roughly the same frequency as before patch but I can't identify any particular behavior that causes it.
(Following up comment #48)

People should test with the 2015-03-18-05-57-50-mozilla-central nightly (the second of two mozilla-central nightlies dates 2015-03-18).
JB:  Please please please test with the mozilla-central nightly mentioned in comment #51, or with today's mozilla-central nightly.
Flags: needinfo?(jwb01)
(In reply to Steven Michaud from comment #52)
> JB:  Please please please test with the mozilla-central nightly mentioned in
> comment #51, or with today's mozilla-central nightly.

ok - testing it now with nightly...
Flags: needinfo?(jwb01)
(In reply to Steven Michaud from comment #52)
> JB:  Please please please test with the mozilla-central nightly mentioned in
> comment #51, or with today's mozilla-central nightly.

Problem still occurs with with the latest mozilla-central nightly.  Now I notice it reliably occurs right after clicking on a plug-in created video -- eg., CNN.com.  After clicking on the video, all keys on the keyboard stop working in the existing window.  The only way to fix is to open a new window or close/reopen firefox.
I just tested with today's mozilla-central nightly on OS X 10.10.2.  I loaded one of the cnn.com videos and then clicked on it.  I didn't see the bug -- afterwards I was able to click on the location bar in the same window and enter text.

Are you still using NoScript?

Do you see the problem in Safe Mode?  https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode?redirectlocale=en-US&redirectslug=Safe+Mode
Flags: needinfo?(jwb01)
(In reply to Steven Michaud from comment #55)
> I just tested with today's mozilla-central nightly on OS X 10.10.2.  I
> loaded one of the cnn.com videos and then clicked on it.  I didn't see the
> bug -- afterwards I was able to click on the location bar in the same window
> and enter text.
> 
> Are you still using NoScript?
> 
> Do you see the problem in Safe Mode? 
> https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-
> mode?redirectlocale=en-US&redirectslug=Safe+Mode


Still using NoScript -- and yes do see the problem in Safe Mode.  Some videos on CNN.com don't cause the issue but I reliably reproduce the problem from CNN.com by clicking on the 'Watch Live TV/CNNgo' video box --usually with a live video playing -- on the mid-right side of the page. (sorry, should have been more specific in my earlier reply.)  After clicking, it will bring up a secondary menu asking which cable provider you have.  by that point, the keyboard stops working.
Flags: needinfo?(jwb01)
Excellent!  Now I can reproduce this bug, too (both in e10s mode and non-e10s mode).  Here's your STR over again, in more detail:

1) http://www.cnn.com/
2) Scroll the page down until you see a "Watch Live TV CNN-go" frame on the right.  Wait until a TV program is playing in the box.  You don't want the "Watch Live TV CNN-go" floating box, always in the lower left on the cnn.com home page.
3) Click once in the middle of the frame from step 2.  Another page will be loaded that asks you to "Select your TV Service Provider".

From this point you see the bug in that browser window.  You can click in text fields like the location bar and the search box, but you can't enter text there (or navigate using the arrow keys).  This remains true (in that browser window) even if you navigate to other pages (for example by clicking on the Back and Forward buttons).

Here's the regression range:

firefox-2015-02-20-03-02-02-mozilla-central
firefox-2015-02-21-03-02-08-mozilla-central

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1b4c5daa7b7a&tochange=5de3af90c494

I will hg bisect to find the patch that triggered this bug.
Status: RESOLVED → REOPENED
Keywords: regressionwindow-wanted
Resolution: FIXED → ---
Whiteboard: [STR in comment #57]
This is bad enough that we're likely going to hold the release for a fix.
status-firefox37: fixed → affected
status-firefox38: fixed → affected
status-firefox39: fixed → affected
> I will hg bisect to find the patch that triggered this bug.

It's my patch for bug 1110888 that triggered this bug :-(

That patch is on the 37 branch and later, so we'll need to deal with this bug (at least on the 37 branch) right away (since the 37 release is coming up soon).  It's simply not an option to back out my patch for bug 1110888 (which would cause even worse trouble).  So I'll be spending the rest of the weekend finding some kind of fix/workaround -- the simpler the better.
Blocks: 1110888
Status: REOPENED → ASSIGNED
Attached patch Second fix, for actual bug (obsolete) — — Splinter Review 
Here's another fix for this bug -- this time for the bug itself.

The reason for the bug is in cross-platform code, here:

http://hg.mozilla.org/mozilla-central/annotate/e730012260a4/dom/base/nsFocusManager.cpp#l818

nsFocusManager::mFocusedContent is cleared without sending any notification (NS_FOCUS_CONTENT or NS_BLUR_CONTENT) to that object.  This causes my patch to bug 1110888 to think that the widget containing that object still has a plugin with the keyboard focus -- and therefore misbehave.

I tend to think this is a bug.  But the comment just above it indicates that it was deliberate.  And I dread the thought of messing with nsFocusManager code.

In any case I found a reasonably simple workaround.  The Element that had the focus gets deleted, and it's possible to register for notification of that.  When that happens I call SetPluginFocused(false) on the owning nsIWidget.

I tried doing this from the existing calls to UnbindFromTree() and DestroyContent(), but these calls are made "too late" -- after the element's primary frame has been zeroed out (so that aElement->GetPrimaryFrame() returns null and GetWidget() doesn't work).

Adding nsIDOMEventTarget to NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED was just a guess.  I don't really understand what this code does.  But I did find parallel code in dom/html/ImageDocument.cpp/h.
Attachment #8581348 - Flags: review?(bugs)
> Adding nsIDOMEventTarget to NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED

I meant nsIDOMEventListener.
Comment on attachment 8581348 [details] [diff] [review]
Second fix, for actual bug

This is using MutationEvents, r-.
Attachment #8581348 - Flags: review?(bugs) → review-
I'm going to have to give this a break for the time being.  I'm too tired.

I'll pick it up again tomorrow.
Another problem that came out when Smaug and I discussed this on IRC:

I call SetPluginFocused(false) (in the node removal handler for HTMLObjectElement/HTMLSharedObjectElement) without first checking that the object was actually focused.  I need to make that check.
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #48)
> (In reply to Steven Michaud from comment #46)
> > Dane, what you describe sounds like a different bug ... though it's really
> > hard to be sure.
> 
> I remember Dane saying on IRC that this was happening after resuming OS X
> from sleep mode and stuff like that. If that's the case, then I believe
> that's bug 1142957, which should be fixed in today's Nightly. 
> 
> Maybe this issue and bug 1142957 are connected in some way? I guess people
> still having this issue might want to try with the March 18th Nightly and
> see if they still have problems.

Just to be clear, I noticed that it only actually happened when I was away long enough that OSX would ask for my password to gain access to the computer again--with Firefox being the last window focused before OSX triggered the password prompt.
nsFocusManger::ContentRemoved() is called by EventStateManager::ContentRemoved():
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#4834

Before the call, IMEStateManager::OnRemoveContent() must be called normally. Then, nsIWidget::SetInputContext() should be called. So, you may be able to catch the context change from "plugin" to another ("enabled", "disabled" or "password") in widget level (perhaps, in nsChildView).
Thanks, Masayuki, I'll give it a try ... tomorrow :-)
And I guess that nsIWidget::SetPluginFocused() isn't necessary.
> And I guess that nsIWidget::SetPluginFocused() isn't necessary.

I think it probably is necessary.  When I was writing my patch for bug 1110888 I found that nsIWidget::SetInputContext() isn't reliable.  Rather that fix that problem (since I was so short of time) I worked around it.

I've also found (I think) that I can rewrite GetWidget() to use nsINode::OwnerDoc() and nsContentUtils::WidgetForDocument() -- that seems to give me the right widget even when called from UnbindFromTree() (even when aElement->GetPrimaryFrame() returns NULL).  It was Smaug who suggested checking out OwnerDoc().

But I'm so tired now that I distrust my ability to concentrate (and dot all the 'i's and cross the 't's).  So I'll pick it up again tomorrow.
(In reply to Steven Michaud from comment #69)
> > And I guess that nsIWidget::SetPluginFocused() isn't necessary.
> 
> I think it probably is necessary.  When I was writing my patch for bug
> 1110888 I found that nsIWidget::SetInputContext() isn't reliable.  Rather
> that fix that problem (since I was so short of time) I worked around it.

Hmm, Gecko for Mac OS X is now the first one which doesn't create widget for plugins. Therefore, IMEStateManager may have some bugs around there.

> I've also found (I think) that I can rewrite GetWidget() to use
> nsINode::OwnerDoc() and nsContentUtils::WidgetForDocument() -- that seems to
> give me the right widget even when called from UnbindFromTree() (even when
> aElement->GetPrimaryFrame() returns NULL).  It was Smaug who suggested
> checking out OwnerDoc().

Yeah, sounds like it will work.

> But I'm so tired now that I distrust my ability to concentrate (and dot all
> the 'i's and cross the 't's).  So I'll pick it up again tomorrow.

Anyway, you should go to bed :-) Tired brain typically creates bad patches.
(In reply to Steven Michaud from comment #60)
> The reason for the bug is in cross-platform code, here:
> 
> http://hg.mozilla.org/mozilla-central/annotate/e730012260a4/dom/base/
> nsFocusManager.cpp#l818
> 
> nsFocusManager::mFocusedContent is cleared without sending any notification
> (NS_FOCUS_CONTENT or NS_BLUR_CONTENT) to that object. 

Yeah, we should fix that.  Other UAs do that, IIRC, and I think the HTML
spec now requires that behavior.
But for this particular bug we want the safest possible fix. So please, no changes to where
blur/focus events are dispatched.
Right, I didn't mean to suggest we fix that in this bug, as I'm sure there
will be some regressions from such a change.  But at some point we need to
do it.
Attached patch Second fix rev1 (obsolete) — — Splinter Review 
Here's a revision that doesn't use mutation events.

I've tested it both with and without e10s, and it works fine in both cases.
Attachment #8581348 - Attachment is obsolete: true
Attachment #8581690 - Flags: review?(bugs)
Comment on attachment 8581690 [details] [diff] [review]
Second fix rev1

> static nsIWidget* GetWidget(Element* aElement)
> {
>   nsIWidget* retval = NULL;
>-  nsIFrame* frame = aElement->GetPrimaryFrame();
>-  if (frame) {
>-    retval = frame->GetNearestWidget();
>+  nsIDocument* ownerDoc = aElement->OwnerDoc();
>+  if (ownerDoc) {
ownerDoc is never null.


>-static void OnFocusBlurPlugin(Element* aElement, bool aFocus)
>+Element* HTMLObjectElement::sLastFocused = NULL; // Weak
Scary, but look like this is used just for comparison.
But anyhow, please make sure the variable doesn't ever point to a deleted object.



>+
>+void
>+HTMLObjectElement::OnFocusBlurPlugin(Element* aElement, bool aFocus)
> {
>   nsIWidget* widget = GetWidget(aElement);
>   if (widget) {
>-    bool value = aFocus;
>-    widget->SetPluginFocused(value);
>+    widget->SetPluginFocused(aFocus);
>+    if (aFocus) {
>+      sLastFocused = aElement;
>+    } else {
>+      sLastFocused = NULL;
NULL?


> {
>+#ifdef XP_MACOSX
>+  // When a page is reloaded (when an nsIDocument's content is removed), the
>+  // focused element isn't necessarily sent an NS_BLUR_CONTENT event. See
>+  // nsFocusManager::ContentRemoved(). This means that a widget may think it
>+  // still contains a focused plugin when it doesn't -- which in turn can
>+  // disable text input in the browser window. See bug 1137229.
>+  //
>+  // This works around the problem. But first check that we actually *were*
>+  // focused (before we were unbound from the tree) -- since (otherwise) some
>+  // other plugin may still be present and focused in the widget.
>+  if (sLastFocused == this) {
>+    OnFocusBlurPlugin(this, false);
>+  }
>+#endif
What if the page goes to bfcache? we don't destroy the DOM then. Can the focus still stay in the plugin?
Please test. 

Can a plugin inside display: none; keep the focus? What is the behavior in the current release? Please test.
Attachment #8581690 - Flags: review?(bugs) → review-
Severity: normal → blocker
>>-static void OnFocusBlurPlugin(Element* aElement, bool aFocus)
>>+Element* HTMLObjectElement::sLastFocused = NULL; // Weak
>
> Scary, but look like this is used just for comparison.  But anyhow,
> please make sure the variable doesn't ever point to a deleted object.

I can't stop sLastFocused from (possibly) pointing to a deleted
object, and I don't need to -- since I only ever check for equality.
That's why I added the following comment:

+  // Weak pointer. May be invalid. Use only for equality checks. NULL if last
+  // action was blur.
+  static Element* sLastFocused;

I'm trying to figure out how to do the tests you requested.
Anyone know how to reliably make a page go into bfcache?
(In reply to Steven Michaud from comment #77)
> I can't stop sLastFocused from (possibly) pointing to a deleted
> object,
Yes you can. Just clear it in dtor, assuming it points to the object being deleted.

> and I don't need to -- since I only ever check for equality.
Sure, but we have had some many sec-critical bugs because of raw pointers, so let's be a bit careful.
(In reply to Steven Michaud from comment #78)
> Anyone know how to reliably make a page go into bfcache?

If a page doesn't have beforeunload or unload event listeners and doesn't have active network connections it should enter bfcache when a new page is loaded.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=ca6d6b665bc1#8802
> Just clear it in dtor, assuming it points to the object being deleted.

But that implies another equality check on a possibly deleted object :-)
No. the object isn't deleted when dtor is called.
But we don't know whether or not sLastFocused has been deleted, and we'll need to compare "this" to that.
If you always check sLastFocused in object/embed dtor and clear it if sLastFocused == this, then
sLastFocused never points to a deleted object.
> and clear it if sLastFocused == this

This implies an equality check with sLastFocused.  But yes, I now see that if you always NULL sLastFocused when sLastFocused == this, sLastFocused will never point to an object that has been deleted.
> What if the page goes to bfcache? we don't destroy the DOM then. Can
> the focus still stay in the plugin? Please test.

I figured out how to test this.  I found out that when a page
containing a focused plugin goes into the bfcache, an NS_BLUR_CONTENT
event does get sent to it (even without my patch).  So we don't need
to worry about that case.

I tested with an interpose library that logs calls to
HTMLObjectElement::HandleFocusBlurPlugin() with aEvent->message ==
NS_FOCUS_CONTENT or NS_BLUR_CONTENT.  It also prints a stack trace for
each call.

Here's the page I tested with (which contains a single Flash plugin):

http://www.adobe.com/devnet/archive/flash/articles/puzzle_game.html

I loaded that page, clicked once on the Flash plugin (to focus it),
then clicked once on the Back button.  The corresponding
HTMLObjectElement() received an NS_BLUR_CONTENT event, and the stack
shows that the event was sent as the page was going into the bfcache
(as it was being "hidden").

One test down, one to go.
Comment on attachment 8582022 [details]
Testcase to make a plugin display: none

Oh hell, it doesn't work from bugzilla.mozilla.org.  I'll have to post it to my people.mozilla.com account.
Attachment #8582022 - Attachment is obsolete: true
Here's my testcase to make a plugin display: none (or more specifically, to make the div that contains it display: none):

http://people.mozilla.org/~stmichaud/bmo/bugzilla1137229/displaynone.html

This also causes an NS_BLUR_CONTENT event to be sent to the corresponding HTMLObjectElement.  So we also don't need to worry about display: none.

I've attached the stack.
I've tested my two testcases in FF 37b7, and found that neither of them triggers this bug.
Attached patch Second fix rev2 (obsolete) — — Splinter Review 
This is the same as my rev1 patch, except I changed every NULL to nullptr and I added code to both destructors to zero sLastFocused if it matches.
Attachment #8581690 - Attachment is obsolete: true
Attachment #8582051 - Flags: review?(bugs)
Smaug found another possible problem, and sent me a suggested fix.  After dinner I'm going to test it and post a revised patch, later tonight.  Smaug says he'll review it tomorrow morning.
Attached file Second fix rev3 (obsolete) — 
Here's my rev2 patch with Smaug's suggested changes (plus a few additional minor changes of my own).  His concern is about what might happen if nsIWidget::SetPluginFocused() is called from UnbindFromTree() -- particularly that Gecko code might run or the event loop might get spun.  I told him this is extremely unlikely, but I'm unable to prove it.  His response was to come up with a way to run nsIWidget::SetPluginFocus() from a script runner -- which only runs when it's safe to run scripts.

I tested this as best I can, and it worked fine.  But the STR for this bug only works when CNN is playing a "live" TV program in the "Watch Live TV CNN-go" frame -- which is generally not late at night, as it now is.

So my final and most important test will need to wait until tomorrow morning.
Attachment #8582051 - Attachment is obsolete: true
Attachment #8582051 - Flags: review?(bugs)
Attachment #8582208 - Flags: review?(bugs)
Comment on attachment 8582208 [details]
Second fix rev3

> static nsIWidget* GetWidget(Element* aElement)
> {
>-  nsIWidget* retval = NULL;
>-  nsIFrame* frame = aElement->GetPrimaryFrame();
>-  if (frame) {
>-    retval = frame->GetNearestWidget();
>+  nsIWidget* retval = nullptr;
>+  nsIDocument* ownerDoc = aElement->OwnerDoc();
>+  if (ownerDoc) {
>+    retval = nsContentUtils::WidgetForDocument(ownerDoc);
>   }

You undid my changes to GetWidget.
OwnerDoc() returns always valid value. No need to null check
(but I had added null check for aElement, but I guess that isn't needed either with this patch)


>+  NS_IMETHOD Run()
>+  {
>+    if (mElement) {
>+      HTMLObjectElement::sLastFocused = mElement;
>+      bool value = true;
>+      mWidget->SetPluginFocused(value);
>+    } else if (!HTMLObjectElement::sLastFocused) {
>+      bool value = false;
>+      mWidget->SetPluginFocused(value);
>+    }
Odd, 'value' variable. Just pass true/false


> #ifdef XP_MACOSX
>   // nsIDOMEventTarget
>   NS_IMETHOD PostHandleEvent(EventChainPostVisitor& aVisitor) override;
>+  // Helper methods
>+  static void OnFocusBlurPlugin(Element* aElement, bool aFocus);
>   static void HandleFocusBlurPlugin(Element* aElement, WidgetEvent* aEvent);
>+  // Weak pointer. NULL if last action was blur.
s/NULL/Null/ or nullptr
Attachment #8582208 - Flags: review?(bugs) → review+
Attached patch Second fix, what I will land (obsolete) — — Splinter Review 
Carrying forward r+.

I tested this morning with the STR from comment #57, and everything went fine.  It's interesting to note that only for calls from UnbindFromTree() did the call to AddScriptRunner() make any difference.  In calls (indirectly) from HandleFocusBlurPlugin(), PluginFocusSetter::Run() always ran immediately (because AddScriptRunner() deemed it safe to do so).  (I didn't see any calls to nsIWidget::SetPluginFocused() from the destructors (directly or indirectly) -- presumably because GetWidget() will always return null at that point.)

> You undid my changes to GetWidget.

Oops, that was inadvertent.  But since neither aElement nor the results of aElement->OwnerDoc() need to be null-checked, I was able to simplify the code still further.

> Odd, 'value' variable. Just pass true/false

This doesn't work -- you get a syntax error from the compiler, probably because nsIWidget::SetPluginFocused() takes a bool&.  So I didn't change my code here.

>>+  // Weak pointer. NULL if last action was blur.
>s/NULL/Null/ or nullptr

Done
Attachment #8582208 - Attachment is obsolete: true
Attachment #8582485 - Flags: review+
Aurora/Beta patch coming up.
Backed out for Werror bustage. I would have let you fix in-place, but you weren't on IRC from what I could tell.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceac43ebc32f

https://treeherder.mozilla.org/logviewer.html#?job_id=7993963&repo=mozilla-inbound
Sigh, dumb mistake.  This will fix it, but mozilla-inbound is now closed.

Carrying forward r+.
Attachment #8582485 - Attachment is obsolete: true
Attachment #8582540 - Flags: review+
> but you weren't on IRC

For future reference, I'm always on IRC when I'm working, in #macdev.  My nick is smichaud.
Approval Request Comment
[Feature/regressing bug #]: Bug/design flaw in nsFocusManager
[User impact if declined]: Bad UI bug.
[Describe test coverage new/current, TreeHerder]: Hand testing by me.
[Risks and why]: Low to moderate. Simple change, but in code that can be tricky.
[String/UUID change made/needed]: none

Carrying forward r+.
Attachment #8582554 - Flags: review+
Attachment #8582554 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/regressing bug #]: Bug/design flaw in nsFocusManager
[User impact if declined]: Bad UI bug
[Describe test coverage new/current, TreeHerder]: Hand testing by me
[Risks and why]: Low to moderate. Simple change, but in code that can be tricky.
[String/UUID change made/needed]: None

Carrying forward r+.
Attachment #8582555 - Flags: review+
Attachment #8582555 - Flags: approval-mozilla-beta?
I assume that FF 37 RC1 is intended to come off the beta branch.  Please let me know if that's wrong, and if I need to prepare a version of my patch for yet another branch.
(In reply to Steven Michaud from comment #103)
> I assume that FF 37 RC1 is intended to come off the beta branch.  Please let
> me know if that's wrong, and if I need to prepare a version of my patch for
> yet another branch.

You only need to land on Beta. We will move the code from mozilla-beta -> mozilla-release before building RC1. If any follow-ups are needed after RC1 they'll need to land on mozilla-beta *and* mozilla-release.
Comment on attachment 8582555 [details] [diff] [review]
Second fix, beta patch

We can't ship with this issue in 37 so we're going to take the fix in the RC. This hasn't landed on m-i (as Steven noted in comment 99). I'm a bit uncomfortable taking the change without it having landed on m-c but I don't see that we have much choice at this point in the schedule. Beta+ Aurora+
Attachment #8582555 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8582554 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Beta is currently closed, so landing there will have to wait a bit.
(In reply to Steven Michaud from comment #107)
> Beta is currently closed, so landing there will have to wait a bit.

mozilla-beta is always closed for the week leading up to the release, I think you're probably fine to push there since you have approval.

I also noticed that we actually already uplifted code to mozilla-release, so you will need to land there as well. Sorry for the bad information earlier =\
(In reply to comment #108)

RyanVM tells me that he'll merge beta to release again, so I don't have to separately land my patch on the release branch.
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/3beddf2d2c8d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
I wasn't able to reproduce the original issue (with STR from comment #57) on Mac OSX 10.9.5 using Firefox Nightly 39.0a1 (2015-03-18, 2015-03-20 and 2015-02-25). In this case, I cannot verify if the bug is fixed on the Firefox 37 and Firefox 38.
Flags: qe-verify+
(In reply to comment #115)

You need to follow the STR in comment #57 exactly.  In particular, note this in step 2 -- "wait until a TV program is playing in the box".  Whenever I tested during US daylight hours, there was always something playing there (though I had to wait for up to a minute for it to appear).  But after about 10pm local time (in the US) I generally didn't see anything playing in the box.

It's also possible, I suppose, that you won't ever see anything playing in the box outside the US -- or even see the box at all.

(I'm in the US, in the Central timezone (which currently uses CDT).)
I'm not able to verify the issue with STR from comment #57: I can not see any TV program playing in the box because I don't have access to any TV Service Provider (I can't create an account for either one of the TV Service Provider).
(In rely to comment #117)

I don't have any accounts, either.  The cnn.com site must behave differently when accessed from outside the US (as I presume you're doing).  You might be able to use a VNC to get around this ... but I don't know exactly how to do that.
This is still happening with OSX 10.10.4 with firefox 39. One test that I did was to use the file menu to create a new FF window. Keyboard input works in the new window but not the original window. I also created a new tab in the keyboard non-responsive window and the keyboard does not work. The only solution is to either create new window or restart firefox to make the keyboard work again.
(In reply to comment #119)

The original bug was fixed.  What you're seeing is almost certainly something new, with similar symptoms.  Please open a new bug and CC me.  Be sure to give detailed steps to reproduce, with specific URLs.  Also be sure to test with a clean profile.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: