Dynamic chrome registration isn't fowarded to content processes

RESOLVED FIXED in mozilla34

Status

()

Core
General
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: jmaher, Assigned: mrbkap)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla34
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [mobile_unittests] [e10s])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

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.

Comment 2

7 years ago
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.

Updated

7 years ago
Blocks: 687201
Created attachment 561911 [details] [diff] [review]
Update content processes when dynamic chrome registration occurs.

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
Component: General → General
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
Blocks: 516752
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
I think we'll probably go with a more add-on-specific approach for this, using bug 990729.
No longer blocks: 516752
tracking-e10s: + → ---
(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.
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

4 years ago
Assignee: nobody → mrbkap
Blocks: 928230
tracking-e10s: --- → +
(Assignee)

Comment 11

4 years ago
Created attachment 8450604 [details] [diff] [review]
jdm's patch

I tested this and this seems to make restartless addons work.
Attachment #561911 - Attachment is obsolete: true
Attachment #8450604 - Flags: review?(benjamin)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 13

4 years ago
Created attachment 8460554 [details] [diff] [review]
patch v2

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.
Attachment #8450604 - Attachment is obsolete: true
Attachment #8460554 - Flags: review?(dtownsend+bugmail)
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.

Comment 17

4 years ago
(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
(Assignee)

Updated

4 years ago
Duplicate of this bug: 928230
https://hg.mozilla.org/mozilla-central/rev/f4bbd83d67e0
Status: NEW → RESOLVED
Last Resolved: 4 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 → ---
(Assignee)

Comment 21

4 years ago
I'm working on the test now.
(Assignee)

Comment 22

4 years ago
Created attachment 8463562 [details] [diff] [review]
Basic test for chrome registration/unregistration.

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)
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+
(Assignee)

Comment 25

4 years ago
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.
(Assignee)

Comment 27

4 years ago
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
Flags: needinfo?(mrbkap)
(Assignee)

Comment 28

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 1049864
https://hg.mozilla.org/mozilla-central/rev/76e8d415df8b
https://hg.mozilla.org/mozilla-central/rev/3f8f7939bbd6
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Flags: qe-verify-

Comment 30

2 years ago
Created attachment 8721552 [details]
MozReview Request: sign file_bug687194.xpi

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.