530633 - [10.6] Firefox Help menu sometimes greyed out the first time it's opened after sleep

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug has been spun off from bug 513048.  My patch for that bug
didn't completely fix the problem.

This bug has the same cause, and more or less the same steps to
reproduce and the same symptoms.  But it's much less severe than bug
513048:

Bug 513048 happened every time you opened the Help menu over the
"same" browser window.  This bug only happens the first time you open
the Help menu.

Here are the steps-to-reproduce from bug 513048 comment #56, lightly
edited to fit this bug:

1) In the Security pref panel, select "require password after sleep or
   screen saver begins".  The interval doesn't matter, but you must be
   prompted for your password (after waking from sleep) to see the
   bug.

2) Start Namoroka and press one key, once.

   (A single browser window opens when you start Namoroka.)

3) Choose Apple : Sleep, then wait about 5 seconds.

4) Press the space key to wake from sleep.

   Enter your password when you're prompted to do so.

5) Use the mouse to open the browser's Help menu.

   The first time you do this, all the items in the menu will be
   disabled (greyed out).

   The problem disappears the second time you open the Help menu.

Comment 1

[Aaron Train [:aaronmt]]

So what exactly is the bug in code?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Better fix/workaround

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 414348 [details]
Better fix/workaround

Turns out my first patch's strategy was correct, but it was triggered
too "late" to fix bug 513048 in every case.

My first patch worked around Apple's bug by correcting the
Carbon-enabled state of the Help menu's items every time the Help menu
opened.  I've now discovered it's better to do this just after each
item have been added to the Help menu, and perhaps also just after a
menu item's enabled state has changed.  (In my brief tests I found it
was only necessary to make this correction in the first case.  But
it's probably safer also to make it in the second case.  And I can't
see this causing any harm.)

Here's a 1.9.2-branch tryserver build made with my patch:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla530633-192branch/bugzilla530633-192branch-macosx.dmg

I'd like everyone who's seen this bug to try it out.  If it passes
your tests I'll ask for review.

I'm going away for Thanksgiving, so I won't be able to do (or see)
anything here until next week.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I cannot test this tryserver build because I'm not able to reproduce the bug with a recent 1.9.2 or trunk build. Please anyone who can reproduce it test this build so it can be fixed for Firefox 3.6.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Henrik, try this revised STR with a recent Namoroka nightly (on OS X
10.6.X).  I find it "works" with today's Namoroka nightly on 10.6.2.

1) In the Security pref panel, select "require password after sleep or
   screen saver begins".  The interval doesn't matter, but you must be
   prompted for your password (after waking from sleep) to see the
   bug.

2) Start Namoroka -- a single browser window should appear.

3) Click once on the browser window's Location bar -- its contents
   should be highlighted.

4) Press an arrow key -- the contents of the Location bar should no
   longer be highlighted.

5) Choose Apple : Sleep, then wait about 5 seconds.

6) Press the space key to wake from sleep.

   Enter your password when you're prompted to do so.

7) Use the mouse to open the browser's Help menu.

   The first time you do this, all the items in the menu will be
   disabled (greyed out).

   The problem disappears the second time you open the Help menu.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> Please anyone who can reproduce it test this build so it can be
> fixed for Firefox 3.6.

Yes!!!  Your silence so far is deafening.

Comment 7

[Matthew N. [:MattN]]

(In reply to comment #4)
> Please anyone who can reproduce it test this build so it can be
> fixed for Firefox 3.6.

I've just tested the build and can no longer reproduce the problem with the STR in the description.  I also verified that the revised STR in comment 5 does not lead to the problem with the tryserver build.

OS X 10.6.2

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 414348 [details]
Better fix/workaround

Thanks, MattN.

On the strength of your results (in addition to my own), I'll ask for
review.

Comment 9

[Josh Aas]

I'm pretty tired of dealing with this problematic code and we only need it for 10.4. I think I'd prefer that we leave this bug on Mac OS X 10.4 and use run-time detection for enabling Cocoa-only event handling on 10.5 (right now that is a compile-time choice). The Cocoa event handling works great without any of this nastiness. It has been baking on trunk for a while now.

We need to decide whether or not this needs to make it into 3.6. It is getting pretty late for major changes and the summary here says "much less severe than bug 513048."

If we aren't going to fix this for 3.6 then we should just not bother doing anything for now since trunk is 10.5+.

Comment 10

[Josh Aas]

That said, thanks for figuring out what is going on here :)

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Yes, dealing with Apple bugs is a pain.

But my fix is narrowly targeted, and I think a lot less risky than
backporting the Cocoa-only menu stuff to 3.6.  The Cocoa-only menu
stuff is only on the trunk, and hasn't been exposed to enough
user-level testing.

So I think we should try to get this patch into 3.6 ... or failing
that into 3.6.1.

I regret not having completely fixed the problem the first time
around.  But we all make mistakes, and its usually best just to
correct them.

Comment 12

[Matthew N. [:MattN]]

I believe this bug and bug 513048 should both be fixed on 1.9.1 and 1.9.2 as it will cause problems for all users who try to check for minor or major updates.  It's the easiest way that people have to update and so it's what I recommend to others when updates are available.

Although bug 513048 fixed the help menu so it only has a problem the first time it's opened, I didn't try a second time (when it didn't work after that bug was fixed) because I was accustomed to it not working. Previously I had to restart the browser or open a new window to be able to open the menu.

Comment 13

[Josh Aas]

From what I can tell, this is not serious enough to block 3.6 at this point. I am requesting blocking though because I want to have a more UI-oriented person make the final decision here.

This is not testable via automated test.

Comment 14

[Joe Drew (not getting mail)]

Utterly sucks, should not block.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Agreed with comment 13 and comment 14; not blocking.

Alex, I added some polish keywords, but over to you to make sure I did it right!

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 414348 [details]
Better fix/workaround

As noted in bug 513048 comment #53, sleeping isn't the only way to
reproduce this bug:  You can also see it after unlocking your computer
from the screensaver, or even after having momentarily triggered the
screensaver without having to log back in.  Turns out I didn't test
this adequately :-(

One way to trigger the screensaver is as follows:

1) In the Desktop & Screen Saver pref panel, click on the Screen Saver
   tab and click on the Hot Corners button.

2) Set one of the "Active Screen Corners" to "Start Screen Saver".

3) Hover the mouse over the screen corner you chose in step 2 -- after
   a second or two the screen saver should start.

   If you then quickly move the mouse again, the screen saver should
   stop without prompting you for a password -- even if you've set
   "Require password immediately after sleep or screen saver begins"
   in the Security pref panel.

   With my current patch, doing so (after having done steps 1-4 from
   my STR in comment #5) tends to "permanently" disable the Help menu
   (not just the first time you open it).

Thanks to Henrik for pointing this out to me while we were in MV -- on
my own computer no less!

I'll post a new patch in my next comment.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix/workaround rev1 (combine previous patches)

Turns out I needed to combine my old and new patches.

This new patch works in all the tests I've performed.  But I also
really need input from other testers.

Particularly Henrik and Marcia :-)

A tryserver build will follow in a few hours.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #17:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla530633-rev1-192branch/bugzilla530633-rev1-192branch-macosx.dmg

Comment 19

[Josh Aas]

Comment on attachment 417818 [details] [diff] [review]
Fix/workaround rev1 (combine previous patches)

>+      BOOL cocoaEnabled;

This is handled correctly later on but please initialize it for future safety.

>+              Boolean carbonEnabled = ::IsMenuItemEnabled(helpMenuRef, index+1);
>+              if (carbonEnabled != cocoaEnabled) {
>+                if (!carbonEnabled) {
>+                  ::EnableMenuItem(helpMenuRef, index+1);
>+                } else {
>+                  ::DisableMenuItem(helpMenuRef, index+1);

Style nit: "index + 1" is easier to read (IMO) and the most commonly used style in Cocoa widget code. Happens a few times here and later on in the patch.

>+    nsAutoString id;
>+    mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::id, id);
>+    if (id.Equals(NS_LITERAL_STRING("helpMenu"))) {

Can you add a function to menu utils called "IsXULHelpMenu" (or something like that) and have it take an "nsIContent*" argument? That way we can avoid duplicating the details of the check.

We should be sure to have QA check the results of this patch if/when we take it for 3.6.1.

Comment 20

[Josh Aas]

Forgot to say - the patch looks fine except for those minor issues, I just want to see it once more to be safe after it is updated. Thanks.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix/workaround rev2 (follow Josh's suggestions)

Here's my revised patch.

And here's a tryserver build made with it:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla530633-rev2-192branch/bugzilla530633-rev2-192branch-macosx.dmg

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 420182 [details] [diff] [review]
Fix/workaround rev2 (follow Josh's suggestions)

Can someone co-ordinate with QA as presumably we've taken this patch on trunk, and renominate for 1.9.2.3 once we've confirmed that it's the fix we want?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> presumably we've taken this patch on trunk

No.  This bug doesn't exist on the trunk.  Neither does bug 513048, whose patch didn't completely fix the original bug.

(Bug 513048's patch *is* already on the 1.9.2 branch, and in FF 3.6.)

To ensure adequate testing, this patch should land on 1.9.2.3 as soon as possible (when the 1.9.2 branch reopens after the 1.9.2.2 release).

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> To ensure adequate testing, this patch should land on 1.9.2.3 as
> soon as possible (when the 1.9.2 branch reopens after the 1.9.2.2
> release).

I'd certainly appreciate help from QA testing this patch.  But nothing
beats user testing :-)

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 420182 [details] [diff] [review]
Fix/workaround rev2 (follow Josh's suggestions)

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 420182 [details] [diff] [review]
Fix/workaround rev2 (follow Josh's suggestions)

This bug still exists on the branch; the patch isn't required on trunk.

Comment 27

[LegNeato]

Comment on attachment 420182 [details] [diff] [review]
Fix/workaround rev2 (follow Josh's suggestions)

a=LegNeato for 1.9.2.8. Please land this on mozilla-1.9.2 default.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 420182 [details] [diff] [review]
Fix/workaround rev2 (follow Josh's suggestions)

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f0a353fe15d7