Closed
Bug 1425818
Opened 5 years ago
Closed 5 years ago
Fix devtools/server/tests/browser/browser_canvasframe_helper_* tests on e10s
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
devtools/server/tests/browser/browser_canvasframe_helper_*.js tests all crashes because of CPOW usage: TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_canvasframe_helper_01.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/highlighters.js:642 - Error: unsafe CPOW usage forbidden https://treeherder.mozilla.org/#/jobs?repo=try&revision=62dd476b2c920c984ee05f362020d4038457425b&selectedJob=150174353
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•5 years ago
|
||
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. I only modified the first test, I'll apply the same pattern to all other tests once we agree on the first.
Comment 3•5 years ago
|
||
mozreview-review |
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. https://reviewboard.mozilla.org/r/208074/#review213888 Looks good to me.
Attachment #8937408 -
Flags: review?(pbrosset) → review+
Comment 4•5 years ago
|
||
I'm curious, would these be better written as Chrome tests instead?
Assignee | ||
Comment 5•5 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #4) > I'm curious, would these be better written as Chrome tests instead? I didn't think about that option. In theory it looks like a better option. I just tried and got into troubles, looks like testing against a chrome document mess up with the CanvasFrameAnonymousContentHelper code. At the end it may be simplier to keep using browser mochitest which is more common in devtools.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•5 years ago
|
||
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. Here is a patch to fix all the canvasframe tests. It is sligthly more complex for test #04 which uses two content scripts because of a change of tab document.
Attachment #8937408 -
Flags: review?(pbrosset)
Comment 9•5 years ago
|
||
mozreview-review |
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. https://reviewboard.mozilla.org/r/208074/#review214414 Looks great to me overall. Mostly I'd like you to try and simplify the EventUtils stuff as suggested below. ::: devtools/server/tests/browser/browser_canvasframe_helper_03.js:101 (Diff revision 3) > + let EventUtils = { > + window: content, > + parent: content, > + _EU_Ci: Components.interfaces, > + _EU_Cc: Components.classes, > + }; > + Services.scriptloader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils); Not sure this is needed at all. I think EventUtils might already be loaded in content. Can you try `let { EventUtils } = content;` ? ::: devtools/server/tests/browser/browser_canvasframe_helper_04.js:37 (Diff revision 3) > - root.appendChild(child); > + root.appendChild(child); > - return root; > + return root; > - }; > + }; > > - info("Building the helper"); > + info("Building the helper"); > - let env = new HighlighterEnvironment(); > + // Bind variables on `this` in order to be accessible in the next content script Please make this comment a bit more detailed. It should mention details like the fact that the global stays alive as long as the tab stays alive, and that `this` is the way to access that global. Also explain that this is required because we reload the page in the tab. ::: devtools/server/tests/browser/browser_canvasframe_helper_04.js:46 (Diff revision 3) > > - info("Get an element from the helper"); > + info("Get an element from the helper"); > - let el = helper.getElement("child-element"); > + this.el = helper.getElement("child-element"); > > - info("Try to access the element"); > + info("Try to access the element"); > - is(el.getAttribute("class"), "child-element", > + is(el.getAttribute("class"), "child-element", I think using `this` everywhere (even if not needed) would make reading the code easier. If everywhere you access the tab global (in this content script and the next) you use `this`, then it's straight forward for readers that this is a thing that survives the page reload. ::: devtools/server/tests/browser/browser_canvasframe_helper_04.js:70 (Diff revision 3) > + // Minimal environment for EventUtils to work. > + let EventUtils = { > + window: content, > + parent: content, > + _EU_Ci: Components.interfaces, > + _EU_Cc: Components.classes, > + }; > + Services.scriptloader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils); Same comment about using `let { EventUtils } = content;` here. ::: devtools/server/tests/browser/browser_canvasframe_helper_05.js:110 (Diff revision 3) > + // Minimal environment for EventUtils to work. > + let EventUtils = { > + window: content, > + parent: content, > + _EU_Ci: Components.interfaces, > + _EU_Cc: Components.classes, > + }; > + Services.scriptloader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils); Same comment than earlier about EventUtils. ::: devtools/server/tests/browser/browser_canvasframe_helper_06.js:98 (Diff revision 3) > + // Minimal environment for EventUtils to work. > + let EventUtils = { > + window: content, > + parent: content, > + _EU_Ci: Components.interfaces, > + _EU_Cc: Components.classes, > + }; > + Services.scriptloader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils); And here.
Attachment #8937408 -
Flags: review?(pbrosset)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #9) > Comment on attachment 8937408 [details] > Bug 1425818 - Stop using CPOW in > devtools/server/tests/browser/browser_canvasframe_helper_*.js. > > https://reviewboard.mozilla.org/r/208074/#review214414 > > Looks great to me overall. Mostly I'd like you to try and simplify the > EventUtils stuff as suggested below. > > ::: devtools/server/tests/browser/browser_canvasframe_helper_03.js:101 > (Diff revision 3) > > + let EventUtils = { > > + window: content, > > + parent: content, > > + _EU_Ci: Components.interfaces, > > + _EU_Cc: Components.classes, > > + }; > > + Services.scriptloader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils); > > Not sure this is needed at all. > I think EventUtils might already be loaded in content. > Can you try `let { EventUtils } = content;` ? `content` is a reference to the website `window` object. It doesn't expose EventUtils. AFAIK, EventUtils is injected into mochitest browser scope by the test harness here: https://searchfox.org/mozilla-central/source/testing/mochitest/browser-test.js#971 It ends up being available in the global scope of browser_*.js scripts, but that's it. Unfortunately, ContentTask doesn't inject it automatically. If that helps I can introduce an helper that inject once and synthesizeMouseDown across all canvasframe tests. > ::: devtools/server/tests/browser/browser_canvasframe_helper_04.js:37 > (Diff revision 3) > > - root.appendChild(child); > > + root.appendChild(child); > > - return root; > > + return root; > > - }; > > + }; > > > > - info("Building the helper"); > > + info("Building the helper"); > > - let env = new HighlighterEnvironment(); > > + // Bind variables on `this` in order to be accessible in the next content script > > Please make this comment a bit more detailed. > It should mention details like the fact that the global stays alive as long > as the tab stays alive, and that `this` is the way to access that global. > Also explain that this is required because we reload the page in the tab. Hum. Yes, this is really complex to follow and explain. I attached a new patch with only one content script, it is easier to follow. Instead of reloading the page via the `browser` element, I reload it via `window.location`. It allows me to have only one content script and not have to workaround scopes.
Comment 12•5 years ago
|
||
mozreview-review |
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. https://reviewboard.mozilla.org/r/208074/#review214606 Thanks for simplifying that test. And yes, you're totally right, I was wrong in thinking that EventUtils was available in the content task. Looks like for RDM we created a small helper (although not shared) here https://searchfox.org/mozilla-central/source/devtools/client/responsive.html/test/browser/browser_touch_simulation.js#195 Might be good to reuse, maybe moving it in our shared head.js file? Or at least in a head.js file you can use in all of these tests easily.
Attachment #8937408 -
Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•5 years ago
|
||
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. Please review my lastest changes: https://reviewboard.mozilla.org/r/208074/diff/4-5/ I moved the helper to shared-head, but it forces to load shared-head (which is in client folder) from server. This is a bit sad, but this is only for test and I think it makes sense to share helpers between server and client. May be shared-head, or at least part of it, should be move somewhere in shared?
Attachment #8937408 -
Flags: review+ → review?(pbrosset)
Comment 15•5 years ago
|
||
mozreview-review |
Comment on attachment 8937408 [details] Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. https://reviewboard.mozilla.org/r/208074/#review214838 Thanks for doing this. I agree that pulling it from client during tests isn't a problem. The only concern is if that gives people the impression that importing across those boundaries is OK, which it's not. But then again, this is only a test ...
Attachment #8937408 -
Flags: review?(pbrosset) → review+
Updated•5 years ago
|
Priority: -- → P3
Comment 16•5 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8da482abb3f5 Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js. r=pbro
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8da482abb3f5
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•