Closed Bug 945781 Opened 6 years ago Closed 6 years ago

Remove ChromePowers.js

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Details

Attachments

(1 file, 3 obsolete files)

From bug 943152, comment 7:
"
chromepowers was meant as a shim for the chrome layer which runs with full privileges.  I recall some issues running full special powers in chrome code while initially developing specialpowers.

I know that specialpowers has evolved for the better, it could be possible we don't need chrome powers anymore.
"

The patch in bug 943152 removes some ChromePowers.js code, if possible, but there is some other usage of it out there:
http://mxr.mozilla.org/mozilla-central/search?string=ChromePowers.js

This bug is about removing those instances, if possible.
It makes me wonder why the DOMWindowCreated event handler isn't called for chrome documents when doing window.open. (That's the stuff in SimpleTest.js to attach specialpowers to those windows)
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowers.js?force=1#131
Ok, I had to add
 content/tests/SimpleTest/specialpowers.js (../specialpowers/content/specialpowers.js)
to jar.mn in testing/mochitest/


In bug89419_window.xul, I had to add:
if ((window.parent !== null) &&
    (window.parent !== undefined) &&
    (window.parent.wrappedJSObject.SpecialPowers) &&
    !(window.wrappedJSObject.SpecialPowers)) {
  window.wrappedJSObject.SpecialPowers = window.parent.SpecialPowers;
} else {
  window.wrappedJSObject.SpecialPowers = new SpecialPowers(window);
}

Then I get this error:
 0:09.85 2 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug89419.xul | uncaught exception - ReferenceError: addMessageListener is not defined at chrome://mochikit/content/tests/SimpleTest/specialpowers.js:23

So it looks like ChromePowers.js is still necessary for some of the tests at least.
Thanks for trying this out. I assume specialpowers isn't attached to the chrome space where that javascript is run.
Yes, that's because of the different logic of how ChromePowers.js is attaching itself.
This stuff could be corrected for specialpowers and basically integrate the chromepowers logic into specialpowers.js.

The benefit would be that all mochitest-* tests use the same specialpowers.js and things like bug 943152 would be circumvented.
The disadvantage would be that specialpowers.js would be more complicated.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Then I get this error:
>  0:09.85 2 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/docshell/test/chrome/test_bug89419.xul |
> uncaught exception - ReferenceError: addMessageListener is not defined at
> chrome://mochikit/content/tests/SimpleTest/specialpowers.js:23

Ok, I now know why this is happening. The specialpowers script is loaded in regular content, not in the frame script sandbox that you get when you use loadFrameScript. Only in that sandbox, addMessageListener is defined.
SpecialPowers is specially written for use in the loadFrameScript sandbox, so be able to use SpecialPowers in regular content, you need to rewrite its methods. That's basically where ChromePowers comes in.
You can't use messageManager.loadFrameScript to inject SpecialPowers into the page, because we have a top level window here.

The solution for this test would be to add an empty shim for addMessageListener (and removeMessageListener) or copy the code over from SpecialPowers.snapshotWindow (because that's the only reason why SpecialPowers is needed for in this test). Let me know what you prefer.

The other tests don't seem to have any need for referencing these specialpowers scripts at all.

The other places where ChromePowers.js is being used, is in browser-chrome tests and marionette. I'm not sure if I can remove those safely, but I could try that in a follow-up patch.
Flags: needinfo?(jmaher)
Attached patch 945781.diff (obsolete) — Splinter Review
Something like this, I mean in case you want to have this one mochitest-chrome with specialpowers.js.
Attachment #8347163 - Attachment is obsolete: true
Attachment #8455678 - Flags: feedback?(jmaher)
Comment on attachment 8455678 [details] [diff] [review]
945781.diff

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

I like how we can remove the subwindow requirement to include chrome powers and all the other special powers files!

::: testing/specialpowers/content/specialpowers.js
@@ +155,5 @@
> +if (typeof window != 'undefined') {
> +  window.addMessageListener = function() {}
> +  window.removeMessageListener = function() {}
> +  window.wrappedJSObject.SpecialPowers = new SpecialPowers(window);
> +}

does this really work for browser-chrome, and all the obscure mochitests that open dialog windows, etc?
Attachment #8455678 - Flags: feedback?(jmaher) → feedback+
Flags: needinfo?(jmaher)
Attached patch 945781.diff (obsolete) — Splinter Review
Small spelling correction in one of the comments, pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=6dfaf632aeed
Attachment #8455678 - Attachment is obsolete: true
Attached patch 945781.diffSplinter Review
I made a stupid mistake in the previous patch, which caused failures, pushed this to try again: https://tbpl.mozilla.org/?tree=Try&rev=e41e1e762a73
Attachment #8456980 - Attachment is obsolete: true
Comment on attachment 8457447 [details] [diff] [review]
945781.diff

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

Ok, this works. The failures that you see in the try server are from the patch in bug 941459.
Attachment #8457447 - Flags: review?(jmaher)
Comment on attachment 8457447 [details] [diff] [review]
945781.diff

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

great stuff.
Attachment #8457447 - Flags: review?(jmaher) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/df278df1524f
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.