Closed Bug 1358070 Opened 7 years ago Closed 7 years ago

browser_docshell_type_editor.js should not rely on a devtools image

Categories

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

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1358070 - register dedicated privileged image for browser_docshell_type_editor.js;
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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
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!
Attachment #8859952 - Flags: feedback?(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.
Attachment #8859952 - Flags: feedback?(ckerschb) → feedback?(gijskruitbosch+bugs)
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.
Attachment #8859952 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
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.
Attachment #8859952 - Attachment is obsolete: true
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 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?
Attachment #8860121 - Flags: review?(gijskruitbosch+bugs) → review+
(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?
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/622dd3df21ca
register dedicated privileged image for browser_docshell_type_editor.js;r=Gijs
https://hg.mozilla.org/mozilla-central/rev/622dd3df21ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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.
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.