Closed Bug 1304531 Opened 8 years ago Closed 8 years ago

Remove some CPOWs from some tests/test infrastructure

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(9 files, 7 obsolete files)

Bug 1304531 - Remove this test as it no longer tests anything we care about and uses shims.
58 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1304531 - Remove CPOW (via shim usage) from the mochitest harness.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage from browser_bug655273.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage from browser_timelineMarkers-01.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage from browser_ua_emulation.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage from browser_ConsoleStorageAPITests.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage from browser_WebRequest.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage in browser_web_protocol_handlers.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1304531 - Remove CPOW usage from browser_newtabwebchannel.js.
58 bytes, text/x-review-board-request
Felipe
: review+
Details 
There has been a pattern of (improperly used) CPOW bugs sitting around causing problems until they bother either RyanVM or another sheriff enough that someone is tracked down to come fix them. In addition, CPOWs are very expensive in terms of performance, and removing them could give us speed improvements on test-running times.

I have a few patches that fix some improper CPOWs and one very (extremely) CPOW-shim usage in the test suite itself. I'll be filing followup bugs to root out more CPOWs in more general ways in a few minutes.
Hey Blake, I'm feeling pretty crushed in reviews with bug 1207696. Is there any chance you could give these to someone else?
Attachment #8793538 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793539 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793540 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793541 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793542 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793543 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793544 - Flags: review?(wmccloskey) → review?(felipc)
Attachment #8793545 - Flags: review?(wmccloskey) → review?(felipc)
Comment on attachment 8793538 [details]
Bug 1304531 - Remove this test as it no longer tests anything we care about and uses shims.

https://reviewboard.mozilla.org/r/80254/#review80790

::: testing/mochitest/document-observer-script.js:10
(Diff revision 1)
> +  observe(aWindow) {
> +    let utils = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIDOMWindowUtils);
> +    let outerID = utils.outerWindowID;
> +    let innerID = utils.currentInnerWindowID;
> +
> +    sendAsyncMessage("browser-test:documentCreated",
> +                     { location: aWindow.location.href,
> +                       outerID, innerID });
> +  },

add a comment explaining that it's ok that we don't differentiate the msg here from the two observers
Attachment #8793538 - Flags: review?(felipc) → review+
Comment on attachment 8793539 [details]
Bug 1304531 - Remove CPOW usage from browser_bug655273.js.

https://reviewboard.mozilla.org/r/80256/#review80792
Comment on attachment 8793539 [details]
Bug 1304531 - Remove CPOW usage from browser_bug655273.js.

https://reviewboard.mozilla.org/r/80256/#review80794
Attachment #8793539 - Flags: review?(felipc) → review+
Comment on attachment 8793540 [details]
Bug 1304531 - Remove CPOW usage from browser_timelineMarkers-01.js.

https://reviewboard.mozilla.org/r/80258/#review80796
Attachment #8793540 - Flags: review?(felipc) → review+
Comment on attachment 8793541 [details]
Bug 1304531 - Remove CPOW usage from browser_ua_emulation.js.

https://reviewboard.mozilla.org/r/80260/#review80798
Attachment #8793541 - Flags: review?(felipc) → review+
Comment on attachment 8793542 [details]
Bug 1304531 - Remove CPOW usage from browser_ConsoleStorageAPITests.js.

https://reviewboard.mozilla.org/r/80262/#review80832
Attachment #8793542 - Flags: review?(felipc) → review+
Comment on attachment 8793543 [details]
Bug 1304531 - Remove CPOW usage from browser_WebRequest.js.

https://reviewboard.mozilla.org/r/80264/#review80834
Attachment #8793543 - Flags: review?(felipc) → review+
Comment on attachment 8793544 [details]
Bug 1304531 - Remove CPOW usage in browser_web_protocol_handlers.js.

https://reviewboard.mozilla.org/r/80266/#review80836

::: uriloader/exthandler/tests/mochitest/browser_web_protocol_handlers.js:41
(Diff revision 1)
>    const handlerSvc = Cc["@mozilla.org/uriloader/handler-service;1"].
>                         getService(Ci.nsIHandlerService);
>    handlerSvc.store(protoInfo);
>  
>    // Middle-click a testprotocol link and check the new tab is correct
> -  let link = browser.contentDocument.getElementById("link");
> +  let link = "#link";

best fix!
Attachment #8793544 - Flags: review?(felipc) → review+
Comment on attachment 8793545 [details]
Bug 1304531 - Remove CPOW usage from browser_newtabwebchannel.js.

https://reviewboard.mozilla.org/r/80268/#review80838
Attachment #8793545 - Flags: review?(felipc) → review+
Attachment #8793539 - Attachment is obsolete: true
Attachment #8793540 - Attachment is obsolete: true
Attachment #8793541 - Attachment is obsolete: true
Attachment #8793542 - Attachment is obsolete: true
Attachment #8793543 - Attachment is obsolete: true
Attachment #8793544 - Attachment is obsolete: true
Attachment #8793545 - Attachment is obsolete: true
Comment on attachment 8796759 [details]
Bug 1304531 - Remove CPOW (via shim usage) from the mochitest harness.

https://reviewboard.mozilla.org/r/82488/#review81192
Attachment #8796759 - Flags: review?(felipc) → review+
Comment on attachment 8796761 [details]
Bug 1304531 - Remove CPOW usage from browser_timelineMarkers-01.js.

https://reviewboard.mozilla.org/r/82492/#review81194
Attachment #8796761 - Flags: review?(felipc) → review+
Comment on attachment 8796762 [details]
Bug 1304531 - Remove CPOW usage from browser_ua_emulation.js.

https://reviewboard.mozilla.org/r/82494/#review81196
Attachment #8796762 - Flags: review?(felipc) → review+
Comment on attachment 8796763 [details]
Bug 1304531 - Remove CPOW usage from browser_ConsoleStorageAPITests.js.

https://reviewboard.mozilla.org/r/82496/#review81198
Attachment #8796763 - Flags: review?(felipc) → review+
Comment on attachment 8796764 [details]
Bug 1304531 - Remove CPOW usage from browser_WebRequest.js.

https://reviewboard.mozilla.org/r/82498/#review81200
Attachment #8796764 - Flags: review?(felipc) → review+
Comment on attachment 8796765 [details]
Bug 1304531 - Remove CPOW usage in browser_web_protocol_handlers.js.

https://reviewboard.mozilla.org/r/82500/#review81202
Attachment #8796765 - Flags: review?(felipc) → review+
Comment on attachment 8796766 [details]
Bug 1304531 - Remove CPOW usage from browser_newtabwebchannel.js.

https://reviewboard.mozilla.org/r/82502/#review81204
Attachment #8796766 - Flags: review?(felipc) → review+
Comment on attachment 8796760 [details]
Bug 1304531 - Remove CPOW usage from browser_bug655273.js.

https://reviewboard.mozilla.org/r/82490/#review81206
Attachment #8796760 - Flags: review?(felipc) → review+
Actually, comment 36 is wrong. We do attempt to access the docShell of the initial tab on construction in the non-e10s case. However, since bug 971685 landed, I don't think this test is really useful anymore.
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86f373004a58
Remove CPOW (via shim usage) from the mochitest harness. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/f52e8a615f55
Remove CPOW usage from browser_bug655273.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/25790f4e8707
Remove CPOW usage from browser_timelineMarkers-01.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/3f0e7a49441f
Remove CPOW usage from browser_ua_emulation.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/f6cb5dc72731
Remove CPOW usage from browser_ConsoleStorageAPITests.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/653796c24e16
Remove CPOW usage from browser_WebRequest.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/7f8b3ffa2ec5
Remove CPOW usage in browser_web_protocol_handlers.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/29cc3589446f
Remove CPOW usage from browser_newtabwebchannel.js. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/9d0a04ae04ce
Remove this test as it no longer tests anything we care about and uses shims. r=Felipe,mconley
(In reply to Blake Kaplan (:mrbkap) from comment #0)
> There has been a pattern of (improperly used) CPOW bugs sitting around
> causing problems until they bother either RyanVM or another sheriff enough
> that someone is tracked down to come fix them. In addition, CPOWs are very
> expensive in terms of performance, and removing them could give us speed
> improvements on test-running times.
> 
> I have a few patches that fix some improper CPOWs and one very (extremely)
> CPOW-shim usage in the test suite itself. I'll be filing followup bugs to
> root out more CPOWs in more general ways in a few minutes.

This is great, thanks mrbkap.

One thing that might be useful for rooting out more CPOW usage in our tests is turning the warnings back on. We disabled them by default in bug 1152864.

It might be worthwhile to disable that pref in a try push, and then do some log munging to try to get a full list of culprit tests.

Then, I wonder if it's feasible to try to disable CPOW support for our tests directory by directory.
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #50)
> It might be worthwhile to disable that pref in a try push, and then do some
> log munging to try to get a full list of culprit tests.

That's basically how I came up with these tests :-) More definitely to come. I'm also keeping an eye on "safe" CPOW usage through the shims to get an idea of how pervasive that is.

> Then, I wonder if it's feasible to try to disable CPOW support for our tests
> directory by directory.

That's an interesting idea. I think it should be possible, especially because we run the tests one directory at a time, but I don't know how to do it.
Depends on: 1307645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: