Closed Bug 1251175 Opened 8 years ago Closed 8 years ago

Intermittent browser_ManifestFinder_browserHasManifestLink.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
e10s + ---
firefox48 --- fixed
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: cbook, Assigned: marcosc)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: intermittent-failure, Whiteboard: btpp-active)

Attachments

(1 file, 4 obsolete files)

0001-Bug-1251175-Removed-dependence-on-CPOW.-r-mconley.patch
19.75 KB, patch
Details | Diff | Splinter Review 
https://treeherder.mozilla.org/logviewer.html#?job_id=22359637&repo=mozilla-inbound

 04:33:14 INFO - 190 INFO TEST-UNEXPECTED-FAIL | dom/manifest/test/browser_ManifestFinder_browserHasManifestLink.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW
Assignee: nobody → mcaceres
Removed dependence on getting contentWindowAsCPOW + simplified the tests.
Attachment #8758119 - Flags: review?(mconley)
Whiteboard: btpp-active
Comment on attachment 8758119 [details] [diff] [review]
Remove getting contentWindowAsCPOW

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

The changes in here look fine, but I don't see where we've skipped using a CPOW... where did that happen?

::: dom/manifest/test/browser_ManifestFinder_browserHasManifestLink.js
@@ +2,4 @@
>  /*global Cu, BrowserTestUtils, is, ok, add_task, gBrowser */
>  "use strict";
>  const { ManifestFinder } = Cu.import("resource://gre/modules/ManifestFinder.jsm", {});
> +const defaultURL = new URL("http://example.org/browser/dom/manifest/test/resource.sjs");

I haven't actually checked if this works, but I do note that nothing else in the tree points to this new path. They all point to the old one. Was an alias made, or is there some kind of redirector? Can I assume that you checked that this URL resolves correctly?

@@ +38,5 @@
>    },
>  }];
>  
> +function makeTestURL({ body }) {
> +  var url = new URL(defaultURL);

let, not var
Flags: needinfo?(mcaceres)
Comment on attachment 8758119 [details] [diff] [review]
Remove getting contentWindowAsCPOW

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

Oh, I see - this relies on the other patch you added to my queue.

::: dom/manifest/test/browser_ManifestFinder_browserHasManifestLink.js
@@ +48,5 @@
>   * Test basic API error conditions
>   */
>  add_task(function*() {
>    const expected = "Invalid types should throw a TypeError.";
> +  for (let invalidValue of [undefined, null, 1, {}, "test"]) {

You did the const thing in the other patch I'm reviewing, and undoing this here. Intentional?

It might just be best to fold these two patches together.
Flags: needinfo?(mcaceres)
Comment on attachment 8758119 [details] [diff] [review]
Remove getting contentWindowAsCPOW

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

I find it odd that this undoes some of the stuff you do in your other patch, but the end result is fine. Maybe consider folding the two patches together before landing. Up to you.
Attachment #8758119 - Flags: review?(mconley) → review+
> I find it odd that this undoes some of the stuff you do in your other patch, but the end result is fine. Maybe consider folding the two patches together before landing. Up to you.

Woopsy... that was not intentional... will merge them as you suggested.
> They all point to the old one. Was an alias made, or is there some kind of redirector?
> Can I assume that you checked that this URL resolves correctly?

The URL resolves correctly. I don't know if there was some recent change or something, but browser_* tests are now supposed to get their support files from /browser/ or they 404. See the last paragraph of:

https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests

This tripped me up when all my tests started returning 404 (even though they worked fine previously o_0). Anyway, the new path fixes it. Something to be aware of, in case it comes up with other folks. 

Thanks again for the prompt and helpful review!
Flags: needinfo?(mconley)
Attached patch Fixes suggested by Mike. (obsolete) — — Splinter Review 

        Attachment #8758119 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  


  

        Attachment #8758998 -
        Attachment is obsolete: true

    
  
Blocks: 1165780

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd665597cff
Removed dependence on CPOW. r=mconley
Keywords: checkin-needed

    
    

    
  
sorry had to back this out since this caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29295555&repo=mozilla-inbound
Flags: needinfo?(mcaceres)

    
    

    
  
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29fc4a93e85c
Backed out changeset 3bd665597cff for causing bc failures

    
    

    
  
(In reply to Marcos Caceres [:marcosc] from comment #17)
> The URL resolves correctly. I don't know if there was some recent change or
> something, but browser_* tests are now supposed to get their support files
> from /browser/ or they 404. See the last paragraph of:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests
> 
> This tripped me up when all my tests started returning 404 (even though they
> worked fine previously o_0). Anyway, the new path fixes it. Something to be
> aware of, in case it comes up with other folks. 
> 

Huh. Good to know, thanks.
Flags: needinfo?(mconley)

    
    

    
  
Merge conflict... rebasing
Flags: needinfo?(mcaceres)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Rebased and fixed merge conflict (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8759003 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/941fd40d73de
Removed dependence on CPOW. r=mconley
Keywords: checkin-needed

    
    

    
  
had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29651943&repo=mozilla-inbound
Flags: needinfo?(mcaceres)

    
    

    
  
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1a5cf90a1ee
Backed out changeset 941fd40d73de for causing perma time out failures in browser_ManifestObtainer_obtain.js

    
    

    
  
Investigating... it's going to be one of those bugs...
Flags: needinfo?(mcaceres)

    
    

    
  
Linuxis are definitely busted. Will try adding a requestLongerTimeout() and see what happens.

    
    

    
  
requestLongerTimeout seemed to do the trick.

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef957b0217bb
Removed dependence on CPOW. r=mconley
Keywords: checkin-needed

    
    

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

          status-firefox50:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: