Closed Bug 1128287 Opened 9 years ago Closed 3 years ago

Intermittent testFindInPage | testFindInPage.js - r == - See following stack:

Categories

(Firefox for Android Graveyard :: Testing, defect, P3)

Product:
Firefox for Android Graveyard
Component:
Testing
Platform:
ARM
Android
Type:
defect

Tracking

(firefox38 fixed)

Status:
RESOLVED INCOMPLETE
Milestone:
Firefox 38
Tracking Flags:
Tracking Status
firefox38 --- fixed

People

(Reporter: philor, Assigned: rricard)

References

Details

(Keywords: intermittent-failure, regression, Whiteboard: [leave open])

Attachments

(4 files, 7 obsolete files)

out
142.15 KB, text/plain
Details
Intermittent testFindInPage | testFindInPage.js
4.38 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
Timeout race condition
2.74 KB, patch
Details | Diff | Splinter Review
Let's do it this time !
902 bytes, patch
Margaret
: review+
Details | Diff | Splinter Review 
      No description provided.
Related to bug 1015395?
You probably have a race condition here. The test is trying to search for a string "Robocop" for example. I'm guessing the |mActions.sendKeys(text);| will send your search string to the FindInPage input field a character at a time. Depending on how slow it is / how large a delay between char entry, FindInPage tries to execute partial searchs.

So the initial test looking for Robocoop won't find that string, but will match other partial occurences of 'r', or 'ro' as indicated by the failures above. (we don't do match case yet).

Take a look, but I'm guessing that your first |assertSelection(document);| fails because *something* is selected, and that test as specified, tries to ensure *nothing* is selected.
Hey robin, not sure of your nick, are you looking at this?
Flags: needinfo?(ricard.robin)
Hum, I should definitely take a look at this indeed. Thanks for pointing it out to me ! I think that relying on the repaint to resolve the promise is not good enough. We should send a message back from Java to JS but I don't really know how to do that for now ... And I still have some things to finish before being able to look at this properly.

Note: you can find me on IRC, my nick is rricard (when I am there ...)
Flags: needinfo?(ricard.robin)
Assignee: nobody → ricard.robin
Hi margaret, I need some information from you ! We discussed with capella on how to fix this. It appears that hooking the promise resolution to MozAfterRepaint introduces an intermittent bug. I was proposing (as you did at first) to hook it to a Java to JS message. As before, I'm not able to see how to do it (if you can give me some references in the MXR, that would be great ! :))

You can see my discussion with capella here: http://logs.glob.uno/?c=mozilla%23mobile&s=4+Feb+2015&e=4+Feb+2015#c416261
Flags: needinfo?(margaret.leibovic)
(In reply to Robin Ricard [rricard] from comment #30)
> Hi margaret, I need some information from you ! We discussed with capella on
> how to fix this. It appears that hooking the promise resolution to
> MozAfterRepaint introduces an intermittent bug. I was proposing (as you did
> at first) to hook it to a Java to JS message. As before, I'm not able to see
> how to do it (if you can give me some references in the MXR, that would be
> great ! :))
> 
> You can see my discussion with capella here:
> http://logs.glob.uno/?c=mozilla%23mobile&s=4+Feb+2015&e=4+Feb+2015#c416261

I think that what you can do is instead of using sendRequest to send the "Test:FindInPage" message, you can use sendRequestForResult, and wait until you get a result in JS before continuing with the test. We may still want to keep the "MozAfterPaint" event listener to make sure the page also paints after we're done entering the text. Perhaps we can use a Promise.all for this.

Using sendRequestForResult will involve modifying testFindInPage to implement NativeEventListener instead of GeckoEventListener:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/NativeEventListener.java#11

You should search in the tree for examples of where we use sendRequestForResult to see the pattern in action.

Also, I must have missed this in my last review, but I think we no longer need the logic in testFindInPage to scroll the page:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testFindInPage.java#88

That might also be contributing to some intermittent issues, so we should remove it if we don't need it.
Flags: needinfo?(margaret.leibovic)
Okay thanks, I'll try to get a patch ASAP !
And we still need to keep the logic to scroll the page: it is used to select next matches and I assert that in the selection.
Ok, hum I have a patch but it doesn't seem to work. i'm trying to see what's happening there but well ... I'll get you on IRC !
Attached file out — 
Flags: needinfo?(margaret.leibovic)

    
    

    
  


  
Don't rely on the repaint only but a return promise from the message.

        Attachment #8562070 -
        Attachment is obsolete: true

        Attachment #8562070 -
        Flags: feedback?(margaret.leibovic)

        Attachment #8562238 -
        Flags: review?(margaret.leibovic)

    
  
Flags: needinfo?(margaret.leibovic)

    
    

    
  


  

        Attachment #8562238 -
        Attachment is obsolete: true

        Attachment #8562238 -
        Flags: review?(margaret.leibovic)

        Attachment #8562762 -
        Flags: review?(margaret.leibovic)

    
    

    
  
(In reply to Robin Ricard [rricard] from comment #65)
> Created attachment 8562762 [details] [diff] [review]
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ed2727fc1c5

I think there's something wrong with your try syntax, as no tests actually ran... 

Also, since this test runs as part of rc1 on 2.3 and rc2 on 4.0, you'll need to make sure to include both of those.
Blocks: 1015395
Keywords: regression

    
    

    
  
Needs to be -b o, since we don't even try to run robocop on debug builds.

    
    

    
  
oh ! sorry ! I didn't payed attention to this ...

    
    

    
  
(In reply to Robin Ricard [rricard] from comment #69)
> here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c22a996d8193

I was wrong, we actually skip this on 2.3, so we don't need to run a test there.

First pass was green on 4.0, I retriggered it a bunch of times to make sure there isn't a pesky orange hiding in there :)

    
    

    
  
Comment on attachment 8562762 [details] [diff] [review]
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ed2727fc1c5

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

Looks good, I just have a few nits. I'd also like to make sure we have a useful error if this ever fails for some reason.

r+ with nits addressed, and we can land an updated version if there aren't any oranges on the try run.

::: mobile/android/base/tests/testFindInPage.java
@@ +10,5 @@
>  import org.mozilla.gecko.Element;
>  import org.mozilla.gecko.R;
>  
>  import org.mozilla.gecko.EventDispatcher;
>  import org.mozilla.gecko.util.GeckoEventListener;

You can remove this import now.

@@ +41,5 @@
>          if (event.equals("Test:CloseFindInPage")) {
>              try {
>                  close.click();
>              } catch (Exception e) {
> +                callback.sendError("FindInPage prompt not opened");

Nice, this is a good use of sendError.

@@ +52,5 @@
>      @Override
>      public void setUp() throws Exception {
>          super.setUp();
>  
> +        EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener)this,

You shouldn't need this cast.

::: mobile/android/base/tests/testFindInPage.js
@@ +41,5 @@
> +    text: text,
> +    nrOfMatches: nrOfMatches
> +  });
> +  let repaintPromise = promiseBrowserEvent(browser, "MozAfterPaint");
> +  return Promise.all([messagePromise, repaintPromise]);

What happens if one of these promises is rejected? Does the test fail with a useful error?

        Attachment #8562762 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
  
ok, I fixed the nits and I'm testing if the error handling works correctly. I'll ping you when I have a patch

    
    

    
  
btw, I should start to work on IntelliJ to avoid those kind of dead imports but I can't figure out how to work inside the git tree with it.

    
    

    
  


  
Rely on multiple ends of find in page promises (Repaint and Java callback)

        Attachment #8562762 -
        Attachment is obsolete: true

        Attachment #8562957 -
        Flags: review?(margaret.leibovic)

    
    

    
  


  
Rely on multiple ends of find in page promises (Repaint and Java callback)

        Attachment #8562957 -
        Attachment is obsolete: true

        Attachment #8562957 -
        Flags: review?(margaret.leibovic)

        Attachment #8562960 -
        Flags: review?(margaret.leibovic)

    
    

    
      
  

    
    

    
  
Comment on attachment 8562960 [details] [diff] [review]
Intermittent testFindInPage | testFindInPage.js

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

Looks good. I can land this for you when the tree re-opens.

        Attachment #8562960 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
      
  

    
    

    
  
(In reply to TBPL Robot from comment #79)
> log:
> https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=1988540
> repository: fx-team
> start_time: 2015-02-12T17:05:21
> who: rvandermeulen[at]mozilla[dot]com
> machine: panda-0286
> buildname: Android 4.0 armv7 API 11+ fx-team opt test robocop-2
> revision: 0b63903a6f41
> 
> TEST-UNEXPECTED-FAIL | testFindInPage | GeckoEventExpecter - blockForEvent
> timeout: Robocop:JS
> 0 ERROR Exception caught during test! -
> junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testFindInPage
> | GeckoEventExpecter - blockForEvent timeout: Robocop:JS
> TEST-UNEXPECTED-FAIL | testFindInPage | Exception caught -
> junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testFindInPage
> | GeckoEventExpecter - blockForEvent timeout: Robocop:JS
> Return code: 1
> 02-12 17:45:02.343 E/GeckoLinker( 4487):
> /data/data/com.adobe.flashplayer/lib/libysshared.so: Text relocations are
> not supported

What?! This push was supposed to fix this problem!

rricard, do you think this is perhaps another issue?
Flags: needinfo?(ricard.robin)

    
    

    
  
This is new to me. the fail is different. Just guessing but I think that this could be triggered by the different timeouts in the findText[1] java method.

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testFindInPage.java#65
Flags: needinfo?(ricard.robin)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Ok, no, doesn't seem like it was that.

> 17:35:10     INFO -  testFindInPage.js | Now waiting for MozAfterPaint event from browser
> 17:35:10  WARNING -  TEST-UNEXPECTED-FAIL | testFindInPage | Exception caught - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testFindInPage | GeckoEventExpecter - blockForEvent timeout: Robocop:JS

In my opinion, there's no repaint triggered. If the repaint doesn't occur we should just wait a given time: Maybe use a Promise.race([repaintPromise, timeoutPromise]) should fix the problem.

Note: the repaint promise existed to ensure we waited long enough. But I can see cases where the repaint promise is registered after the emission of the MozAfterRepaint

    
    

    
  
Patch coming soon !

    
    

    
  
Okay ! Simple fix: I was creating the repaint promise after the message promise. This was wrong. Now I'm creating it before. No need for Promise.race for now I think.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix the timeout by MozAfterPaint (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8564105 -
        Flags: review?(margaret.leibovic)

    
    

    
  
Can't push a try right now: tree is closed ...

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/bab1f360bf44

Thanks for fixing this, it was driving me crazy :)
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox38:
          --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

    
    

    
  
Wait ! We still have the second part of the bug to take care of !
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Sorry, Ryan. Soon enough we will have a fix!
Whiteboard: [leave open]

    
    

    
  
(In reply to Robin Ricard [rricard] from comment #90)
> Here's a try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1412677c7079

Thanks. Let me know when it finishes and I can retrigger the test runs for you if you don't have the permissions to do that.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Ok, I'll see what I can do. I think I'll introduce the race condition anyway...
Flags: needinfo?(ricard.robin) → needinfo?(margaret.leibovic)

    
    

    
  
(In reply to :Margaret Leibovic from comment #93)
> (In reply to Robin Ricard [rricard] from comment #90)
> > Here's a try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1412677c7079
> 
> Thanks. Let me know when it finishes and I can retrigger the test runs for
> you if you don't have the permissions to do that.

And looking at that try run, this failure is in there.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Huh. Still failing after the backout?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Yep, if you want to neutralize it you should back out the bug that introduced the intermittent failure.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
As pointed out on IRC, MozAfterPaint seems to have disappeared from the mobile codebase. It may be never emitted, at all. https://dxr.mozilla.org/mozilla-central/search?q=MozAfterPaint+path%3Amobile&case=true

    
    

    
  
Trying to blame a change on this, I found out that I'm the only one to use that on mobile. So I'm just going to read tons of logs to see what's wrong ...

    
    

    
  


  
That's the best I could do with my knowledge. Hope it does the job ...

        Attachment #8564105 -
        Attachment is obsolete: true

        Attachment #8564105 -
        Flags: review?(margaret.leibovic)

        Attachment #8564553 -
        Flags: review?(margaret.leibovic)

    
    

    
  
And the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eece2f7a930
Flags: needinfo?(margaret.leibovic)

    
    

    
  
Ok, I don't know what's going on here ... I'm trying everything but I end up stuck everytime ... :/
Flags: needinfo?(margaret.leibovic)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Bah, this is now perma-red across all trees :(

    
    

    
  
It was because of bug 1014113, now backed out. I'm responsible for both so I'll fix them ASAP ! :s
Flags: needinfo?(margaret.leibovic)

    
    

    
      
  

    
    

    
  
Comment on attachment 8564553 [details] [diff] [review]
Timeout race condition

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

Do you have another try run to go along with this?

And can you explain why we need this timeout? Is the issue that we're just not getting a MozAfterPaint event sometimes? Do you know why would that happen?

    
    

    
  
Nope, but it seems to be attached with bug 1014113. I'll retry intensively that with this patch applied on a 4.0 Emulator on my machine (where the perma rc2 was found).

Yep, I thought that MozAfterPaint wasn't happening ... But it doesn't seem that's the issue (even if the logs are saying this to me: "Waiting for a MozAfterPaint then ... timeout !") I may need your expertise in logs to see if you spot something else causing this ...
Flags: needinfo?(margaret.leibovic)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Hey Robin, I'm sorry I haven't had time to help investigate this. I think we should consider disabling this test again if we can't come up with a solution in the near future.
Flags: needinfo?(margaret.leibovic)

    
    

    
  
Comment on attachment 8564553 [details] [diff] [review]
Timeout race condition

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

Clearing review until we have a patch that's consistently green on try.

        Attachment #8564553 -
        Flags: review?(margaret.leibovic)

    
    

    
      
  

    
    

    
  
Yep, ok, I'll do it. Sorry for not being really active those last days but I started a new part-time job and it'll be harder for me to contribute unfortunately.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Disable testFindInPage (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8573287 -
        Flags: review?(margaret.leibovic)

    
    

    
  
Comment on attachment 8573287 [details] [diff] [review]
Disable testFindInPage

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

Looks good, thanks. We should also uplift this to aurora.

        Attachment #8573287 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
  
(In reply to :Margaret Leibovic from comment #147)
> Comment on attachment 8573287 [details] [diff] [review]
> Disable testFindInPage
> 
> Review of attachment 8573287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks. We should also uplift this to aurora.

Please check in this latest patch to disable this test.
Keywords: checkin-needed

    
    

    
  
this failed to apply patching file mobile/android/base/tests/robocop.ini
Hunk #1 FAILED at 30
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/tests/robocop.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1128287---Disable-testFindInPage.patch
Flags: needinfo?(ricard.robin)
Keywords: checkin-needed

    
    

    
  
Comment on attachment 8573287 [details] [diff] [review]
Disable testFindInPage

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

::: mobile/android/base/tests/robocop.ini
@@ +40,2 @@
>  # disabled on 2.3
>  skip-if = android_version == "10"

Also, please comment out this skip-if -- otherwise it will apply to the previous test, testFilterOpenTab: oops!

    
    

    
      
  

    
    

    
  
Yes, doing that asap !
Flags: needinfo?(ricard.robin)

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Disable testFindInPage (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8573287 -
        Attachment is obsolete: true

        Attachment #8574714 -
        Flags: review?(margaret.leibovic)

    
    

    
  
(In reply to Geoff Brown [:gbrown] from comment #150)
> Comment on attachment 8573287 [details] [diff] [review]
> Disable testFindInPage
> 
> Review of attachment 8573287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/robocop.ini
> @@ +40,2 @@
> >  # disabled on 2.3
> >  skip-if = android_version == "10"
> 
> Also, please comment out this skip-if -- otherwise it will apply to the
> previous test, testFilterOpenTab: oops!

Thanks, gbrown! I'm embarrassed that I missed that.

    
    

    
  
Comment on attachment 8574714 [details] [diff] [review]
Disable testFindInPage

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

::: mobile/android/base/tests/robocop.ini
@@ +36,5 @@
>  skip-if = android_version == "10"
>  [testFilterOpenTab]
> +# [testFindInPage] # see bug 1128287
> +# # disabled on 2.3
> +# skip-if = android_version == "10"

You can just remove these skip-if lines altogether, instead of commenting them out.

        Attachment #8574714 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  

        Attachment #8574714 -
        Attachment is obsolete: true

        Attachment #8575274 -
        Flags: review?(margaret.leibovic)

    
    

    
  
Side note: I'll be back in a few weeks to work on this ! ;)

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8575274 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I needed to disable testFindInPage on Android 4.3 for the same type of failures; I noticed the disabling patch here never landed, so disabled testFindInPage across all versions in bug 1140148.

    
    

    
  
(In reply to Geoff Brown [:gbrown] from comment #172)
> I needed to disable testFindInPage on Android 4.3 for the same type of
> failures; I noticed the disabling patch here never landed, so disabled
> testFindInPage across all versions in bug 1140148.

Thanks, Geoff! Sorry for letting this slip through the cracks :(

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3

    
    

    
  
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 3 years ago
Resolution: --- → INCOMPLETE

    
  
Product: Firefox for Android → Firefox for Android Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: