Closed Bug 889752 Opened 11 years ago Closed 11 years ago

Permanent Orange: TEST-UNEXPECTED-FAIL | test-recent-menu.js | test_move_message, test_delete_message, test_archive_message

Categories

(Thunderbird :: Folder and Message Lists, defect)

Product:
Thunderbird
Component:
Folder and Message Lists
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird24+ fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 26.0
Tracking Flags:
Tracking Status
thunderbird24 + fixed

People

(Reporter: standard8, Assigned: aceman)

References

Details

(Keywords: intermittent-failure)

Attachments

(4 files, 4 obsolete files)

focus log
11.97 KB, text/x-log
Details
[checked in] patch - keep menus open v3
9.53 KB, patch
aceman
: review+
Details | Diff | Splinter Review
[checked in] patch - focus menus v2
2.45 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Avoid another suspicion
1.15 KB, patch
standard8
: review+
aceman
: feedback+
Details | Diff | Splinter Review 
Regression from bug 876347:

Test Failure: a != b: '' != 'true'.
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/folder-display/test-recent-menu.js | test-recent-menu.js::test_move_message

(I suspect the other messages may be follow-on).
Note that there is a patch in bug 876347 to address this failure on other platforms. But it was not enough for Linux.
Assignee: nobody → acelists
Whiteboard: [free to take if you have a patch idea]
Attached patch WIP patch (obsolete) — — Splinter Review 
Aryx, could I have a try run with this patch, mozmill only? Thanks
Flags: needinfo?(archaeopteryx)
Great, what gets the test to reliably pass locally does nothing for the try server :(
Attached patch patch v2 - keep menus open (obsolete) — — Splinter Review 
While the patch does not seem to fix the problem, I'd still like to get some of the changes reviewed. Conceptually I do not like that we open and close the menu and only then look at its (generated) elements. I think that is not safe universally. Is there a guarantee that after closing of a menu the elements are still there? I am not sure. So I'd like to keep the menu open while we inspect its elements and close it after that. This patch allows that, while not changing the current behaviour for tests that do not want that. But I think there are more tests that would like this feature.
Attachment #771018 - Attachment is obsolete: true
Attachment #771450 - Flags: review?(mconley)
Attachment #771450 - Flags: review?(mbanner)
Attachment #771450 - Attachment description: patch v2 → patch v2 - keep menus open
Attached patch patch - focus menus (obsolete) — — Splinter Review 
This patch makes the test pass on my machine locally. I don't know why it does not help on try servers. It explicitly focuses the menuitems being clicked on. Is this good for anything? (To be applied on top of the first patch.)
Attachment #771456 - Flags: feedback?(mconley)
Attachment #771456 - Flags: feedback?(mbanner)
Though this failure is permanent on build servers but I can not reproduce this failure on local.

aceman, cloud you please take a log with "NSPR_LOG_MODULES=Focus:5,WidgetFocus:5" on failure case if you can reproduce this failure? The log will be very helpful to solve the failure.
Attached file focus log — 
This is the log you requested with a clean tree. The test fails like this:

SUMMARY-UNEXPECTED-FAIL | test-recent-menu.js | test-recent-menu.js::test_move_message
  EXCEPTION: Popup never opened at action depth 0; id=mailContext-fileHereMenu, state=showing
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       _click_menus test-window-helpers.js 1004
       test_move_message test-recent-menu.js 40
       Runner.prototype.wrapper frame.js 585
       Runner.prototype._runTestModule frame.js 655
       Runner.prototype.runTestModule frame.js 701
       Runner.prototype.runTestFile frame.js 534
       runTestFile frame.js 713
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-recent-menu.js | test-recent-menu.js::test_delete_message
  EXCEPTION: Popup never opened! id=mailContext, state=showing
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       _click_menus test-window-helpers.js 965
       test_delete_message test-recent-menu.js 83
       Runner.prototype.wrapper frame.js 585
       Runner.prototype._runTestModule frame.js 655
       Runner.prototype.runTestModule frame.js 701
       Runner.prototype.runTestFile frame.js 534
       runTestFile frame.js 713
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-recent-menu.js | test-recent-menu.js::test_archive_message
  EXCEPTION: Popup never opened! id=mailContext, state=showing
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       _click_menus test-window-helpers.js 965
       test_archive_message test-recent-menu.js 97
       Runner.prototype.wrapper frame.js 585
       Runner.prototype._runTestModule frame.js 655
       Runner.prototype.runTestModule frame.js 701
       Runner.prototype.runTestFile frame.js 534
       runTestFile frame.js 713
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-PASS | test-recent-menu.js::teardownModule
make: *** [mozmill-one] Error 1
Attachment #776595 - Flags: feedback?(hiikezoe)
aceman, thanks for the log. But this log is slightly different what I was expecting...
What I was expecting a log that including mozmill's log like this:

TEST-PASS | /home/zoe/hg/comm-central/mail/test/mozmill/folder-display/test-recent-menu.js | test-recent-menu.js::setupModule
TEST-START | /home/zoe/hg/comm-central/mail/test/mozmill/folder-display/test-recent-menu.js | test_move_message
1354350464[19544b0]: Window 2a020a0 Hidden [Currently: 2769010 2769010]
1354350464[19544b0]:   Hide Window: chrome://messenger/content/msgAccountCentral.xul
1354350464[19544b0]:   Focused Window: chrome://messenger/content/messenger.xul
1354350464[19544b0]:   Active Window: chrome://messenger/content/messenger.xul
1354350464[19544b0]: Window 29e3380 Hidden [Currently: 2769010 2769010]

Anyway what I can tell from the log you post is some focus events did no seem to be handled before starting test_move_message.

From your log:

-1219864832[b722b0c0]: Window b72906b0 Shown [Currently: b2632e30 b2632e30]
-1219864832[b722b0c0]: Shown Window: mailbox:///var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/aaafolder1?number=0
-1219864832[b722b0c0]:  Focused Window: chrome://messenger/content/messenger.xul
-1219864832[b722b0c0]: <<SetFocus begin>>
-1219864832[b722b0c0]: <<SetFocus end>>
-1219864832[b722b0c0]: <<SetFocus begin>>
-1219864832[b722b0c0]: <<SetFocus end>>
-1219864832[b722b0c0]: <<SetFocus begin>>
-1219864832[b722b0c0]: <<SetFocus end>>
-1219864832[b722b0c0]: OnContainerFocusOutEvent [af97f7b0]
-1219864832[b722b0c0]: Window b2632e30 Lowered [Currently: b2632e30 b2632e30]
-1219864832[b722b0c0]:   Lowered Window: chrome://messenger/content/messenger.xul
-1219864832[b722b0c0]:   Active Window: chrome://messenger/content/messenger.xul
-1219864832[b722b0c0]: <<Blur begin>>
-1219864832[b722b0c0]: Element tree has been blurred
-1219864832[b722b0c0]: Done with container focus out [af97f7b0]
-1219864832[b722b0c0]: OnContainerFocusInEvent [af97f7b0]
-1219864832[b722b0c0]: Window b2632e30 Raised [Currently: 0 0]
-1219864832[b722b0c0]:   Raised Window: b2632e30 chrome://messenger/content/messenger.xul
-1219864832[b722b0c0]: <<Focus begin>>
-1219864832[b722b0c0]: Element tree has been focused
-1219864832[b722b0c0]:  from window
-1219864832[b722b0c0]:  [Newdoc: 1 FocusChanged: 0 Raised: 1 Flags: 0]
-1219864832[b722b0c0]:   SetFocus 0 [af97f7b0]
-1219864832[b722b0c0]:   widget now has focus in SetFocus() [af97f7b0]
-1219864832[b722b0c0]: Update Caret: 0 1
-1219864832[b722b0c0]: Events sent from focus in event [af97f7b0]
-1219864832[b722b0c0]: Window b72906b0 Hidden [Currently: b2632e30 b2632e30]
-1219864832[b722b0c0]:   Hide Window: mailbox:///var/SSD/TB-hg/tbird-bin/mozilla/_tests/mozmill/mozmillprofile/Mail/Local%20Folders/aaafolder1?number=0
-1219864832[b722b0c0]:   Focused Window: chrome://messenger/content/messenger.xul
-1219864832[b722b0c0]:   Active Window: chrome://messenger/content/messenger.xul

I suppose this is the part of test_move_message. "OnContainerFocusOutEvent" and "OnContainerFocusInEvent" should have been handled before the test. Those events prevents popup menu open.
aceman, I would like to ask you one more thing. Your log is *timeout* case. I would like to see *Test Failure: a != b: '' != 'true'* case.
Flags: needinfo?(acelists)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> aceman, thanks for the log. But this log is slightly different what I was
> expecting...
> What I was expecting a log that including mozmill's log like this:
Sorry, I was redirecting mozmill log to another file. I can produce it if needed.

> Anyway what I can tell from the log you post is some focus events did no >seem to be handled before starting test_move_message.
Maybe that is why my "patch - focus menus" helps me.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #54)
> aceman, I would like to ask you one more thing. Your log is *timeout* case.
> I would like to see *Test Failure: a != b: '' != 'true'* case.

I understand you see that on thunderbird-trunk. But I think I do not get it locally. For me it fails sooner. The menu is not properly opened and it fails there. On Tb-trunk it somehow does not notice the menu was not opened and then fails accessing the menu items. But it is the same menu so the cause may 
be the same.
Flags: needinfo?(acelists)
Attached patch Possible fix (obsolete) — — Splinter Review 
Somebody please send this patch to try server.
I suppose this patch will solve the failure case of "Test Failure: a != b: '' != 'true'".
Comment on attachment 776967 [details] [diff] [review]
Possible fix

Aryx, could you please push this to try server, mozmill tests only.
Attachment #776967 - Flags: feedback?(archaeopteryx)
There seems to be no change with the patch in the try run.
My assumption was wrong. I supposed _build method in folderWidgets.xml is not processed at the failure time.

The only other possibility I can imagine is :

the following .noSelect in folderWidgets.xml check does not properly work

 if (this._parentFolder.noSelect)
   menuitem.setAttribute("disabled", "true");

Is this really possible?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #73)
> The only other possibility I can imagine is :
> 
> the following .noSelect in folderWidgets.xml check does not properly work
> 
>  if (this._parentFolder.noSelect)
>    menuitem.setAttribute("disabled", "true");

I meant
  if (!recentFolders.length)
    menu.setAttribute("disabled", "true");
Bug 876347 triggered these problems, where the lines you say have been added. But as the failures only happen on Linux, it should be some focus/timing problem again. And in my local tests, the whole menu does not popup even before execution comes to the code you mention.
Comment on attachment 771456 [details] [diff] [review]
patch - focus menus

Review of attachment 771456 [details] [diff] [review]:
-----------------------------------------------------------------

This has bitrotted, but after fixing the bitrot it does seem to help for me!
Attachment #771456 - Flags: feedback+
Comment on attachment 771456 [details] [diff] [review]
patch - focus menus

Review of attachment 771456 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, though it won't help if the OS is doing focus-stealing prevention, and something other than TB takes focus.

I recall that being an issue one time - during a test run, some OS dialog would pop up during the test run and take focus. The OS's focus-stealing code prevented TB from taking focus again programmatically (it had to be triggered via a user action). Maybe that's what we're experiencing here - focus-stealing protection.

If so, I wonder if there's a way to turn that off on our tinderbox machines?
Attachment #771456 - Flags: feedback?(mconley) → feedback+
Comment on attachment 771450 [details] [diff] [review]
patch v2 - keep menus open

Review of attachment 771450 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +919,5 @@
>       *     node has an attribute with that name and value.  We click whatever we
>       *     find.  We throw if we don't find what you were asking for.
> +     * @param aKeepOpen  If set to true the popups are not closed after last click.
> +     *
> +     * @return  An array of popup elements that still were left open.

We should add that this returns an empty array if aKeepOpen is not true.
Attachment #771450 - Flags: review?(mconley) → review+

        Attachment #771450 -
        Attachment is obsolete: true

        Attachment #771450 -
        Flags: review?(mbanner)

        Attachment #779950 -
        Flags: review+

    
  
Keywords: checkin-needed
Whiteboard: [free to take if you have a patch idea] → [free to take if you have a patch idea] [check in only the marked patch]

    
  
Blocks: 871266

    
    

    
  
Comment on attachment 776967 [details] [diff] [review]
Possible fix

Looks like this didn't work.

        Attachment #776967 -
        Attachment is obsolete: true

        Attachment #776967 -
        Flags: feedback?(archaeopteryx)

    
    

    
  
Comment on attachment 779950 [details] [diff] [review]
[checked in] patch - keep menus open v3

https://hg.mozilla.org/comm-central/rev/6b2de324d78f

        Attachment #779950 -
        Attachment description: patch - keep menus open v3 [ready for checkin] → [checked in] patch - keep menus open v3 [ready for checkin]

    
  
Keywords: checkin-needed
Whiteboard: [free to take if you have a patch idea] [check in only the marked patch] → [free to take if you have a patch idea]

    
    

    
  
Comment on attachment 771456 [details] [diff] [review]
patch - focus menus

I'm guessing this one is obsolete.

        Attachment #771456 -
        Attachment is obsolete: true

        Attachment #771456 -
        Flags: feedback?(mbanner)

    
    

    
  
Comment on attachment 771456 [details] [diff] [review]
patch - focus menus

It is not obsolete it just does work 100%. It worked for me and mkmelin, but not on the try server. So either I scrape it or refine it.

        Attachment #771456 -
        Attachment is obsolete: false

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

          tracking-thunderbird24:
          --- → +

    
    

    
  


  
This still seems to help me (and Magnus) locally and it didn't seem to cause any problems on TB-try so it may be worth to include.
To be applied on top of bug 871266.

        Attachment #771456 -
        Attachment is obsolete: true

        Attachment #781877 -
        Flags: review?(mbanner)

    
    

    
  
Comment on attachment 781877 [details] [diff] [review]
[checked in] patch - focus menus v2

Review of attachment 781877 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, lets go for it. r=Standard8

        Attachment #781877 -
        Flags: review?(mbanner) → review+

    
    

    
  
Comment on attachment 781877 [details] [diff] [review]
[checked in] patch - focus menus v2

https://hg.mozilla.org/comm-central/rev/9b3eb173c798

        Attachment #781877 -
        Attachment description: patch - focus menus v2 → [checked in] patch - focus menus v2

    
    

    
  


  
I am now suspecting some other tests updated MRMTime property.
This patch resets MRMTime before running test-recent-menu.js.

Someone please push this patch to try server.
Thank you.

    
    

    
  
Comment on attachment 784804 [details] [diff] [review]
Avoid another suspicion

But the patch
https://bug876347.bugzilla.mozilla.org/attachment.cgi?id=765535 did not change the logic of the number of recent menu items so if this would be a problem, it should have cropped up even before my change. But your change seems sound anyway.

        Attachment #784804 -
        Attachment description: Avoid another suspicionss → Avoid another suspicion

        Attachment #784804 -
        Flags: feedback+

    
    

    
  
Aryx, we'd need a trybuild with the patch from comment 134. All systems but Mozmill only. Thanks.
Flags: needinfo?(archaeopteryx)

    
    

    
  
Opps, forgot to comment:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=2a1b8350ea9a
Flags: needinfo?(archaeopteryx)

    
    

    
  
Looking good so far for Linux 32 Mozmill. I've just triggered some respins of just the mozmill tests for that.

Linux 64 failed due to tree bustage, that should be fixed now, so I pushed afresh for that:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=64b81cec5d12

    
    

    
  
Yo ho! Thanks Mark! test-recent-menu.js passed on all respins now!

    
    

    
  
Comment on attachment 784804 [details] [diff] [review]
Avoid another suspicion

Linux 64 has passed with flying green as well :-)

Therefore I'm going to say r+ to this and get it landed. The only thing I'd say, is that it would be nice to have a comment in the file as to why we're doing this (just like the check-in comment, but quicker to find) - but I'll do that on checkin.

    
  

        Attachment #784804 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/comm-central/rev/259c50bae39e

Thanks to both aceman & Hiro for working on the fix to this.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

    
  

        Attachment #779950 -
        Attachment description: [checked in] patch - keep menus open v3 [ready for checkin] → [checked in] patch - keep menus open v3

    
    

    
  
Yay! We did it!

    
  
Whiteboard: [free to take if you have a patch idea]

    
  

        Attachment #776595 -
        Flags: feedback?(hiikezoe)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: