Closed Bug 687194 Opened 10 years ago Closed 7 years ago
Dynamic chrome registration isn't fowarded to content processes
24.11 KB, patch
|Details | Diff | Splinter Review|
7.01 KB, patch
|Details | Diff | Splinter Review|
58 bytes, text/x-review-board-request
we try to access chrome://mochitests/content/.../file.js from a test case and load it via loadFrameScript, but we throw an warning in the console indicating we cannot load the specified script. This is most likely due to the fact that we dynamically register tests.jar as chrome://mochitests when we click the 'run tests' button.
Benjamin said that we should just try to avoid doing that when I last queried him on this issue.
Avoid doing what, dynamic registration? Its not great but I'm not sure what the alternative is here. Do we not forward dynamic chrome registration stuff? That's going to break a lot of the restartless addons which can now register chrome packages.
Ah, apparently I wasn't clear last time then. Yeah, no updates take place after the initial chrome dump. That's something that should be fixed.
This patch might fix the problem here. I need to do some more reading to figure out if there are more possible registrations that could occur that aren't being handled.
Assignee: nobody → josh
Target Milestone: --- → Firefox 9
Version: Trunk → Firefox 10
Product: Fennec Graveyard → Core
Summary: browser chrome tests fail to loadFrameScript from tests.jar → Dynamic chrome registration isn't fowarded to content processes
Whiteboard: [mobile_unittests] → [mobile_unittests] [e10s]
Target Milestone: Firefox 9 → ---
Version: Firefox 10 → Trunk
This may be important for some addons to work correctly, but I'm not actively working on it.
Assignee: josh → nobody
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
I think we'll probably go with a more add-on-specific approach for this, using bug 990729.
(In reply to Bill McCloskey (:billm) from comment #7) > I think we'll probably go with a more add-on-specific approach for this, > using bug 990729. Hey Bill, how about mochitest's that rely on content scripts registered by the mochitest harness being available to the content process? I filed bug 1022040 (and then duped it to this bug) regarding that particular use-case. Is it something we want to support as well?
For my own benefit, the registration happens here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/chrome-harness.js#397 I guess we could take something like Josh's patch, but I'm worried about how this will affect sandboxing. We'll be able to map the file to a path, but we won't actually be able to open it. However, I guess we could also take Josh's patch. It doesn't make the sandboxing situation any worse, and it would make this a lot easier.
I tested this and this seems to make restartless addons work.
Attachment #8450604 - Flags: review?(benjamin) → review?(dtownsend+bugmail)
Comment on attachment 8450604 [details] [diff] [review] jdm's patch Review of attachment 8450604 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this will only forward registration of "content" packages. Restartless add-ons can also register "locale", "skin" and "override". This patch could really use some tests.
Attachment #8450604 - Flags: review?(dtownsend+bugmail) → review-
I think that this is a more complete patch, though I'm at a complete loss as for how to test it. In particular, loading chrome:// in the URL bar loads the resulting page in the chrome process and my attempts to load visible things in the content process only caused tears and assertions. Mossop said he had an idea of how to test this, I'm hoping he'll comment here to elucidate. That being said, with this patch, I believe that we'll correctly handle "dynamic" additions and removals of chrome mappings.
So my idea is predicated on being able to load a frame script in tests, but browser-chrome tests should be able to do that? All the test needs to do is create a directory, put a chrome.manifest in it with some registrations then call "Components.manager.addBootstrappedManifestLocation" for that directory. Then in a frame script attempt to resolve chrome urls that should have been registered with nsIChromeRegistry.convertChromeURL
Comment on attachment 8460554 [details] [diff] [review] patch v2 Review of attachment 8460554 [details] [diff] [review]: ----------------------------------------------------------------- This approach looks good. I'd still like to see tests if we can though
Attachment #8460554 - Flags: review?(dtownsend+bugmail) → review+
I had to work around this bug here: http://hg.mozilla.org/mozilla-central/file/06ac51c2b8a8/dom/tests/browser/browser_test_new_window_from_content.js#l160 So I think a pretty simple test case would be to have some Mochitest support file that is a content script that echoes some message to it. Then load the frame script into some remote browser, send a message to it, and pass if we get the echo back. Something like that, anyway.
(In reply to Mike Conley (:mconley) from comment #16) > I had to work around this bug here: > > http://hg.mozilla.org/mozilla-central/file/06ac51c2b8a8/dom/tests/browser/ > browser_test_new_window_from_content.js#l160 > > So I think a pretty simple test case would be to have some Mochitest support > file that is a content script that echoes some message to it. Then load the > frame script into some remote browser, send a message to it, and pass if we > get the echo back. > > Something like that, anyway. We have this, which might be helpful. I've been thinking of adding mm message support here for a different issue. http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest-e10s-utils-content.js
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
To be clear my r+ was predicated on also getting a test written or an explanation of why it isn't possible to test this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm working on the test now.
For some reason, my extension didn't actually run in the mozbrowser frame, but the test passes. I couldn't make it a chrome test because of exceptions in SpecialPowers setting up the mozbrowser. I also looked at making a browser-chrome test, but was defeated trying to open a remote-process window pointed to a script that I wanted. Ugh.
Attachment #8463562 - Flags: review?(dtownsend+bugmail)
The extension in attachment 8463562 [details] [diff] [review] can be found at <https://github.com/mrbkap/e10sbug/tree/1f16b390c0165d13bf383e4c2647c1fd149d375e>.
Comment on attachment 8463562 [details] [diff] [review] Basic test for chrome registration/unregistration. Review of attachment 8463562 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Should we check the code for the add-on into the tree?
Attachment #8463562 - Flags: review?(dtownsend+bugmail) → review+
I think it should be enough to check the .xpi into the tree. If somebody wants to modify it, they'll be able to unzip it, make their changes, and repackage it.
Something from here landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2a00dc11cb And was then backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf012084abd for mochitest-5 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=45285082&tree=Mozilla-Inbound
Pushed again. I had forgotten to disable the new test (ironically) for e10s. Because the test creates its own child process, there's no need to run it explicitly in the child process. Moreover, it uses the AddonManager directly and that can't happen in the child process. https://hg.mozilla.org/integration/mozilla-inbound/rev/76e8d415df8b
The test was failing on Android for some reason. I'm disabling it there. I'll file a followup on investigating why it was failing. https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8f7939bbd6
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Review commit: https://reviewboard.mozilla.org/r/35703/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35703/
You need to log in before you can comment on or make changes to this bug.