Closed Bug 1278418 Opened 8 years ago Closed 8 years ago

Very frequent test_subresources_prompts.html | checking button0 default - got false, expected true

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: KWierso, Assigned: dragana)

References

Details

(Keywords: intermittent-failure, Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

bug_1278418.patch
1.99 KB, patch
dragana
: review+
Details | Diff | Splinter Review 
I will take a look.
Assignee: nobody → dd.mozilla
Flags: needinfo?(dd.mozilla)
I manage to reproduce it only once out of 20-30 runs locally.
The test is failing in part I have not change at all. I added a new task  runTestAuth at the end of the test and it is always failing in task runTest. 
Timing issue :(
The only way I can provoke this error is if I move focus somewhere else while the test is running.
Short update:

For the auth dialog tabModal is disable and they work just fine, even if it is out of focus.  

For the other prompts tested in the same test tab_modal must be enabled. If the window is not in focus in line:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/test/chromeScript.js#105
result is false and should be true.

I do not know our prompts dialog that well...I do not know why is like that :)
Hmm, interesting. If the only error is "checking button0 default - got false, expected true", that means none of the other buttons are being set as default.


Does removing the |window.sizeToContent();| you added make this problem go away?

At the bottom of checkPromptState() in toolkit/components/prompts/test/prompt_common.js, you'll see that we currently ignore checking the focused element on Linux ("Focus seems missing or wrong on Linux"). It might be interesting to see what the focused element is anyway, comparing a normal run with a failure. [It's just a string by the time the info reaches checkPromptState(), if you want to poke at the real element see getPromptState() in chromeScript.js.]

Also, it occurs to me that the way we wait for the prompt to open [handlePrompt() in chromeScript.js] isn't necessarily waiting for the prompt to load and initialize... It's a little surprising this isn't already failing, but it might be interesting to see if commonDialogOnLoad() has run yet for the prompt when the test fails. Maybe try (for debugging) setting a window.isInitialized flag there, and have checkPromptState() complain if it's not set?

Although... Eww. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/dialog.xml#162

This is setting the default button and focus from a setTimeout, after the dialog's |load| event. So that could very easilty be what's going wrong here. Again, can you verify with some debugging that this code hasn't yet run when the test fails? That would confirm if this is the problem or not.
Whiteboard: [necko-active]
The prompt that is failing does not use commonDialogOnLoad at all. The part that is failing is the original part of the test (use to be call test_bug625187.html).
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/content/tabprompts.xml

button is set as default at:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/CommonDialog.jsm#161

and by the time it suppose to be check the attribute "default" exist but the value is false.
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/dialog.xml#327 is not call (this would delete attribute)

This is what i see locally, I am trying to get a try run to fail, but it does not want :) 


I commented out window.sizeToContent(); in the try run (but that try run still have not failed...). Locally the issue is reproducibly without that line.
I fot
I got it to fail. So behavior is the same as a locally. It can be reproduce if during the test the focus changes (somewhere else is clicked). Probably needs to change focus multiple time, because of timing.

So if focus is change the default button gets a "default" attribute at:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/CommonDialog.jsm#161

but until the tests checks the attribute value, the value is false and should be true. The attribute still exist.

At the same time the focus is null.


I do not know this code well and I cannot decide if this important or not.
A work around for the test could be turn this check into todo if focus is missing.
Flags: needinfo?(dolske)
Would it help to use SimpleTest.waitForFocus()?

This is the #2 unfixed intermittent orange right now.
Flags: needinfo?(dd.mozilla)
Attached patch bug_1278418.patch (obsolete) — — Splinter Review 
Flags: needinfo?(dolske)
Flags: needinfo?(dd.mozilla)

        Attachment #8764559 -
        Flags: review?(dolske)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #21)
> Would it help to use SimpleTest.waitForFocus()?
> 
> This is the #2 unfixed intermittent orange right now.

tried that, but it did not work. Thanks.

    
    

    
  
Comment on attachment 8764559 [details] [diff] [review]
bug_1278418.patch

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

Still baffled as to why this is failing, but fair to work around it at this point (and there's already a similar workaround below it...).

r+ assuming you can remove the origIsTabModal thing, if that's actually needed I'd like to look again because I'm missing something. :)

::: toolkit/components/prompts/test/prompt_common.js
@@ +81,2 @@
>      is(promptState.defButton1, expectedState.defButton == "button1", "checking button1 default");
>      is(promptState.defButton2, expectedState.defButton == "button2", "checking button2 default");

Go ahead and put all 3 of these promptState.defButton# checks into the else block. They don't really seem like valid checks when this problem occurs (and if we ever added a test were button 1/2 was expected to be the default, would presumably fail on Linux too).

Also, with the todo(false) above, s/failes/fails/ and add a reference to this bug #.

::: toolkit/components/prompts/test/test_subresources_prompts.html
@@ +114,5 @@
>    // cross-origin subresources load
>  
>    // Force parent to not look for tab-modal prompts, as they're not
>    // used for auth prompts.
> +  var origIsTabModal = isTabModal;

I don't understand how saving and restoring isTabModal helps -- you're restoring it right at the end of the test, and so I'd not expect it to have any effect.

        Attachment #8764559 -
        Flags: review?(dolske) → review+

    
    

    
      
  

    
    

    
  


  

        Attachment #8764559 -
        Attachment is obsolete: true

        Attachment #8764832 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eccaeee0a605
Add TODO for missing default button when focus is missing. r=dolske
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/eccaeee0a605
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox50:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Just a heads up:

I've fixed (or so I hope!) the underlying test failure that was causing this over in bug 1265077, which essentially removes the workaround that was added here.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: