Closed Bug 1147720 Opened 8 years ago Closed 8 years ago

Intermittent browser_urlbarEnterAfterMouseOver.js,browser_urlbarStop.js | Test timed out | Found a tab after previous test timed out

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal
Points:
2

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Flags:
Tracking Status
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: ttaubert)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

test screenshot
604.02 KB, image/png
Details
0001-Bug-1147720-Fix-intermittent-waitForDocLoadAndStopIt.patch, v2
4.71 KB, patch
mak
: review+
Details | Diff | Splinter Review
Attached image test screenshot — 
Same 404 error in the OSX screenshot.

11:40:27 INFO - 622 INFO checking window state
11:40:27 INFO - 623 INFO Entering test
11:40:27 INFO - 624 INFO TEST-PASS | browser/base/content/test/general/browser_urlbarStop.js | location bar reflects loaded page
11:40:27 INFO - 625 INFO TEST-PASS | browser/base/content/test/general/browser_urlbarStop.js | location bar reflects loading page
11:40:27 INFO - 626 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarStop.js | Test timed out - expected PASS
11:40:27 INFO - 627 INFO MEMORY STAT vsize after test: 1718292480
11:40:27 INFO - 628 INFO MEMORY STAT residentFast after test: 599085056
11:40:27 INFO - 629 INFO MEMORY STAT heapAllocated after test: 226143216
11:40:27 INFO - 630 INFO TEST-OK | browser/base/content/test/general/browser_urlbarStop.js | took 45628ms
11:40:27 INFO - 631 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarStop.js | Found a tab after previous test timed out: http://mochi.test:8888/whatever.html - expected PASS
Flags: needinfo?(ttaubert)
Summary: Intermittent browser_urlbarStop.js | Test timed out | Found a tab after previous test timed out → Intermittent browser_urlbarEnterAfterMouseOver.js,browser_urlbarStop.js | Test timed out | Found a tab after previous test timed out
browser_urlbarStop.js should use the waitForDocLoadAndStopIt in head.js rather than providing its own

Still browser_urlbarEnterAfterMouseOver.js could indicate waitForDocLoadAndStopIt is slightly bogus (but I think we knew already)
(In reply to Marco Bonardo [::mak] from comment #8)
> browser_urlbarStop.js should use the waitForDocLoadAndStopIt in head.js
> rather than providing its own

I mostly copied that implementation because browser_urlbarStop.js needs to call |content.stop()| whereas the function in head.js calls |req.cancel(Cr.NS_ERROR_FAILURE)|. Couldn't get the test to work with the head.js version.
Flags: needinfo?(ttaubert)
we might need to figure out why. In the worst case, add an option for the different behavior or refactor... I think someone soon or later will notice the dupe and remove it, and then the test will fail with no apparent reason.
Fair enough, I'll try to figure out the timeouts and will combine the functions then.
Moved browser_urlbarStop.js' waitForDocLoadAndStopIt() implementation to head.js. The other tests work with content.stop() as well.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8583746 - Flags: review?(mak77)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59b01d53a4a5
Iteration: --- → 39.3 - 30 Mar
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Might fix intermittent browser_aboutHome.js timeouts too, btw.
Comment on attachment 8583746 [details] [diff] [review]
0001-Bug-1147720-Fix-intermittent-waitForDocLoadAndStopIt.patch

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

Some questions

::: browser/base/content/test/general/head.js
@@ +371,5 @@
>   * @return promise
>   */
>  function waitForDocLoadAndStopIt(aExpectedURL, aBrowser=gBrowser.selectedBrowser) {
>    function content_script() {
> +    const {interfaces: Ci, utils: Cu} = Components;

nit: I'd keep these as let, to avoid naming conflicts

@@ +377,4 @@
>  
>      let progressListener = {
> +      onStateChange(webProgress, req, flags, status) {
> +        if (flags & Ci.nsIWebProgressListener.STATE_START) {

I'd probably keep around some dumps for investigation of future failures, unless it's a problem to have them in the content process.

@@ +381,4 @@
>            wp.removeProgressListener(progressListener);
> +
> +          /* Hammer time. */
> +          content.stop();

I noticed you removed 2 things:
1. nsIRequest.cancel
2. isTopLevel
I think they were mostly overzealous actions, I wonder if you removed them cause they are just not strictly needed, or cause they were blocking some use-cases.
If they are just overzealous, I'd probably keep them.

@@ +408,5 @@
>    }
>  
> +  return new Promise(resolve => {
> +    const MSG = "test:waitForDocLoadAndStopIt";
> +    const SCRIPT = content_script.toString().replace("{MSG}", MSG);

why do we need this complication? The previous code was looking simpler to me and doesn't look like we can reuse this code in multiple places. that is, I'd just hardcode MSG again.
(In reply to Marco Bonardo [::mak] from comment #17)
> >      let progressListener = {
> > +      onStateChange(webProgress, req, flags, status) {
> > +        if (flags & Ci.nsIWebProgressListener.STATE_START) {
> 
> I'd probably keep around some dumps for investigation of future failures,
> unless it's a problem to have them in the content process.

Alright, can re-add the ones from the previous implementation I guess.

> > +          content.stop();
> 
> I noticed you removed 2 things:
> 1. nsIRequest.cancel

Yes, the load is now stopped by content.stop(). nsIRequest.cancel() doesn't work for browser_urlbarStop.js but content.stop() works for all other tests as well.

> 2. isTopLevel

Oops, that got lost when experimenting. Will add it back.

> > +  return new Promise(resolve => {
> > +    const MSG = "test:waitForDocLoadAndStopIt";
> > +    const SCRIPT = content_script.toString().replace("{MSG}", MSG);
> 
> why do we need this complication? The previous code was looking simpler to
> me and doesn't look like we can reuse this code in multiple places. that is,
> I'd just hardcode MSG again.

AAaaaallllright.

        Attachment #8583746 -
        Attachment is obsolete: true

        Attachment #8583746 -
        Flags: review?(mak77)

        Attachment #8583823 -
        Flags: review?(mak77)

    
    

    
      
  

    
    

    
  
Comment on attachment 8583823 [details] [diff] [review]
0001-Bug-1147720-Fix-intermittent-waitForDocLoadAndStopIt.patch, v2

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

::: browser/base/content/test/general/head.js
@@ +390,5 @@
> +          /* Hammer time. */
> +          content.stop();
> +
> +          /* Let the parent know we're done. */
> +          sendAsyncMessage("Test:WaitForDocLoadAndStopIt", {uri: chan.originalURI.spec});

nit: whitespace inside braces for readability?

        Attachment #8583823 -
        Flags: review?(mak77) → review+

    
    

    
      
  

    
    

    
      
  
Blocks: 1102935

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

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

          status-firefox39:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

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