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)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1425818 - Stop using CPOW in devtools/server/tests/browser/browser_canvasframe_helper_*.js.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
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 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 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+
I'm curious, would these be better written as Chrome tests instead?
(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 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 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)
(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 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 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 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+
Priority: -- → P3
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
https://hg.mozilla.org/mozilla-central/rev/8da482abb3f5
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.