1358070 - browser_docshell_type_editor.js should not rely on a devtools image

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

DevTools are moving away from mozilla-central to be hosted on a separate repository. 

Currently image/test/browser/browser_docshell_type_editor.js is using a devtools image [1]. We should either switch to another image or register an image specifically for this test.

[1] chrome://devtools/content/framework/dev-edition-promo/dev-edition-logo.png

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: register dedicated image for browser_docshell_type_editor.js

I wanted to try and register a new image for this test. Had to create a dedicated jar.mn for this so I'm not sure it's worth the trouble? Otherwise I can simply switch the image for another one.

Let me know what you think!

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8859952 [details] [diff] [review]
register dedicated image for browser_docshell_type_editor.js

(In reply to Julian Descottes [:jdescottes] from comment #1)
> Created attachment 8859952 [details] [diff] [review]
> register dedicated image for browser_docshell_type_editor.js
> 
> I wanted to try and register a new image for this test. Had to create a
> dedicated jar.mn for this so I'm not sure it's worth the trouble? Otherwise
> I can simply switch the image for another one.

Thanks for fixing, but I honestly don't know. Gijs is probably the right person who can help you.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8859952 [details] [diff] [review]
register dedicated image for browser_docshell_type_editor.js

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

This will ship the image to everyone even though we only need it for a test.

You can register a temporary manifest using the Components manager - Components.manager.addBootstrappedManifestLocation , and remove it at the end of the test. That way the image will really only be there for the test.

Comment 4

[:Gijs (he/him)]

To be clear, then you will want a .manifest file, not a jar.mn file. Should just have a single skin line, without the "%" prefix you use in jar.mn.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1358070 - register dedicated privileged image for browser_docshell_type_editor.js;

The test image/test/browser/browser_docshell_type_editor.js used to rely on a devtools image.
Devtools are moving away from mozilla central, so here we register a new image dedicated for
this test.

Review commit: https://reviewboard.mozilla.org/r/132144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132144/

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the feedback Gijs! 

I tried to follow your advice in the last patch, looks like the test passing locally.
Try ongoing at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3de41bb4808799acfaed556fc4fe6f25ddeec48

Comment 7

[:Gijs (he/him)]

Comment on attachment 8860121 [details]
Bug 1358070 - register dedicated privileged image for browser_docshell_type_editor.js;

https://reviewboard.mozilla.org/r/132144/#review135274

This is really nice! Thanks so much for doing this.

I don't suppose I could convince you to port this fix for bug 1279236, too? That's for basically exactly the same problem but in caps/tests/mochitest/test_bug292789.html .

::: image/test/browser/browser_docshell_type_editor.js:6
(Diff revision 1)
> +const MANIFEST_PATH = "browser/image/test/browser/browser_docshell_type_editor";
> +
> +function getManifestDir() {
> +  let file = Components.classes["@mozilla.org/file/directory_service;1"]
> +      .getService(Components.interfaces.nsIProperties)
> +      .get("CurWorkD", Components.interfaces.nsILocalFile);
> +
> +  MANIFEST_PATH.split("/").forEach(part => file.append(part));
> +  return file;
> +}

It should be possible to use `getTestFilePath()` here instead of hardcoding this test's subdir (image/test/browser/) and relying on the working directory. Can you use that instead?

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8860121 [details]
Bug 1358070 - register dedicated privileged image for browser_docshell_type_editor.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132144/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to :Gijs from comment #7)
> Comment on attachment 8860121 [details]
> Bug 1358070 - register dedicated privileged image for
> browser_docshell_type_editor.js;
> 
> https://reviewboard.mozilla.org/r/132144/#review135274
> 
> This is really nice! Thanks so much for doing this.
> 

Thanks for the review ! 

> ::: image/test/browser/browser_docshell_type_editor.js:6
> (Diff revision 1)
> > +const MANIFEST_PATH = "browser/image/test/browser/browser_docshell_type_editor";
> > +
> > +function getManifestDir() {
> > +  let file = Components.classes["@mozilla.org/file/directory_service;1"]
> > +      .getService(Components.interfaces.nsIProperties)
> > +      .get("CurWorkD", Components.interfaces.nsILocalFile);
> > +
> > +  MANIFEST_PATH.split("/").forEach(part => file.append(part));
> > +  return file;
> > +}
> 
> It should be possible to use `getTestFilePath()` here instead of hardcoding
> this test's subdir (image/test/browser/) and relying on the working
> directory. Can you use that instead?

Done, thanks.

> I don't suppose I could convince you to port this fix for bug 1279236, too?
> That's for basically exactly the same problem but in
> caps/tests/mochitest/test_bug292789.html .
> 

I'd be happy to but I'm stuck when I try to use Components.manager.removeBootstrappedManifestLocation. 

I don't think we have access to Components in chrome mochitests? I tried going through the SpecialPowers utils but I can't find a way to achieve the same result. Is there a way to do this in a chrome mochitest, or should we try to use a different type of test?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/622dd3df21ca
register dedicated privileged image for browser_docshell_type_editor.js;r=Gijs

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/622dd3df21ca

Comment 12

[:Gijs (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> (In reply to :Gijs from comment #7)
> > I don't suppose I could convince you to port this fix for bug 1279236, too?
> > That's for basically exactly the same problem but in
> > caps/tests/mochitest/test_bug292789.html .
> > 
> 
> I'd be happy to but I'm stuck when I try to use
> Components.manager.removeBootstrappedManifestLocation. 
> 
> I don't think we have access to Components in chrome mochitests? I tried
> going through the SpecialPowers utils but I can't find a way to achieve the
> same result. Is there a way to do this in a chrome mochitest, or should we
> try to use a different type of test?

Hm, I haven't tried, but I think with this code:

https://dxr.mozilla.org/mozilla-central/source/docshell/test/navigation/test_contentpolicy_block_window.html#31,32

var componentManager = SpecialPowers.wrap(SpecialPowers.Components).manager
                                    .QueryInterface(Ci.nsIComponentRegistrar);

(not sure you need the QI)

it should be possible without having to switch the test over to another framework.