Closed Bug 1133761 Opened 9 years ago Closed 9 years ago

Try to improve addon shims to help with the e10s conversion process

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Details

Attachments

(2 files, 4 obsolete files)

The idea Bill had is that we should try to find a way to give useful warnings that helps to ease the pain of porting an addon to e10s.
tracking-e10s: --- → ?
Assignee: nobody → gkrizsanits
So I just went through Bug 1048164 as start. Apart from the warning I think the more broader goal is to ease the pain as much as possible for add-on developers, when porting their add-on to e10s. So I will just go through the issues here Anthony encountered and see what we can do about them.

I also encourage everyone to join and throw in any annoying issues around e10s that gave hard time to someone and think we should do a better work there, to make it more obvious how the issue should be handled.

I guess Dave and Bill might have a few good ones...

Anyway, here it goes:

Frame scripts:
- Featuring that they run per frames not per tabs (important for observers) *maybe warm here for observers?*
- they cannot access files *warning here* what other limitations do we know?
- reaching message manager from frame scripts is hard and counter-intuitive:
  first reaching the chrome global, then we can access the mm from there
  function chromeGlobalForDocument(document)
  {
    return document
      .defaultView
      .QueryInterface(Ci.nsIInterfaceRequestor)
      .getInterface(Ci.nsIWebNavigation)
      .QueryInterface(Ci.nsIDocShellTreeItem)
      .rootTreeItem
      .QueryInterface(Ci.nsIInterfaceRequestor)
      .getInterface(Ci.nsIContentFrameMessageManager);
  }
  + this only works for the child process, so about:newtab will throw for this
  *warning could be nice for the frame script against about:newtab like chrome frames*
  but I think an API for reaching the mm is also needed...

- Then there was this solution... I don't quite get it... is this documented? should we have an API for this for addons? (see Bug 1048164 Comment 32)
let obs = {
  observe: function(doc, topic, data) {
    if (!doc.defaultView) return;
    if (doc.defaultView.top !== content) return;
    // do stuff
  }
};
Services.obs.addObserver(obs, "document-element-inserted", false);
addEventListener("unload", () => {
  Services.obs.removeObserver(obs, 'document-element-inserted')
}, false);

- registering nsIContentPolicy... do we have a doc for this? *warning for attempts from the wrong process*

- http-on-modify-request and other -on- listeners are parent process only observable => not from frame script *warning here as well*

- The nsIContentPolicy shouldCall() method receives a <browser> when e10s is off, but an nsIDomWindow when it's on... not sure what to do with this, documentation is needed for sure, but I'm not sure I know what is going on here. Also, I might know bugs where this might be the issue, so could someone explain this to me?

So the million dollar questions is ofc, what else is missing from the list, but would like to hear feedback on these ones as well, if you guys have some time for it...
Flags: needinfo?(wmccloskey)
Flags: needinfo?(dtownsend)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> - reaching message manager from frame scripts is hard and counter-intuitive:
>   first reaching the chrome global, then we can access the mm from there
>   function chromeGlobalForDocument(document)
>   {
>     return document
>       .defaultView
>       .QueryInterface(Ci.nsIInterfaceRequestor)
>       .getInterface(Ci.nsIWebNavigation)
>       .QueryInterface(Ci.nsIDocShellTreeItem)
>       .rootTreeItem
>       .QueryInterface(Ci.nsIInterfaceRequestor)
>       .getInterface(Ci.nsIContentFrameMessageManager);
>   }
>   + this only works for the child process, so about:newtab will throw for
> this
>   *warning could be nice for the frame script against about:newtab like
> chrome frames*

I think you can just do:

document.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
                    .getInterface(Ci.nsIDocShell)
                    .QueryInterface(Ci.nsIInterfaceRequestor)
                    .getInterface(Ci.nsIContentFrameMessageManager);

And from code it looks like that should work regardless of the process. Note also https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/RemotePageManager which I really need to get around to blogging about which I expect to ease the transition for a lot of in-content pages.

> - Then there was this solution... I don't quite get it... is this
> documented? should we have an API for this for addons? (see Bug 1048164
> Comment 32)
> let obs = {
>   observe: function(doc, topic, data) {
>     if (!doc.defaultView) return;
>     if (doc.defaultView.top !== content) return;
>     // do stuff
>   }
> };
> Services.obs.addObserver(obs, "document-element-inserted", false);
> addEventListener("unload", () => {
>   Services.obs.removeObserver(obs, 'document-element-inserted')
> }, false);

The complexity here is largely because this runs in a frame script. You could use a process script instead but I added a DOM event that corresponds to the notification in bug 1139099 so you can now do this in a frame script and it has exactly the same effect:

addEventListener("DOMDocElementInserted", (event) => {
  let doc = event.target;
  // do stuff
});
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #2)

> I think you can just do:
> 
> document.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
>                     .getInterface(Ci.nsIDocShell)
>                     .QueryInterface(Ci.nsIInterfaceRequestor)
>                     .getInterface(Ci.nsIContentFrameMessageManager);
> 
> And from code it looks like that should work regardless of the process. Note
> also

I would not have an idea why that works... still seems to be quite "magical"

> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> RemotePageManager which I really need to get around to blogging about which
> I expect to ease the transition for a lot of in-content pages.

Oh, man... This would have help me a lot when I wrote tests for domainPolicy,
we really should have an e10s knowledge base somewhere :(

> 
> The complexity here is largely because this runs in a frame script. You
> could use a process script instead but I added a DOM event that corresponds
> to the notification in bug 1139099 so you can now do this in a frame script
> and it has exactly the same effect:
> 
> addEventListener("DOMDocElementInserted", (event) => {
>   let doc = event.target;
>   // do stuff
> });

Awesome, thanks for that. It already helped me once. But what do you mean by process script? A jsm in the child? Or something else? I have never heard that term...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> Oh, man... This would have help me a lot when I wrote tests for domainPolicy,
> we really should have an e10s knowledge base somewhere :(
> 

Oh, this looks a lot nicer already than I though https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox I should re-visit these pages more frequently...
I think https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_chrome_scripts and https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_frame_scripts are perfect sources of when do we need warnings. Also, I think we should warning for every time someone is using shims for various things we think are only temporary (so all the shim code really?)

In the meanwhile why I was reading though these lists I realized that we seem to be missing a public API like ContentTask and a similar one let's say ChromeTask for things like file io from content side. This should go into a separated bug (Bug 1146298).
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> > The complexity here is largely because this runs in a frame script. You
> > could use a process script instead but I added a DOM event that corresponds
> > to the notification in bug 1139099 so you can now do this in a frame script
> > and it has exactly the same effect:
> > 
> > addEventListener("DOMDocElementInserted", (event) => {
> >   let doc = event.target;
> >   // do stuff
> > });
> 
> Awesome, thanks for that. It already helped me once. But what do you mean by
> process script? A jsm in the child? Or something else? I have never heard
> that term...

Bill added them recently in bug 1133594. Basically like frame scripts but instead of running once per frame they instead run once per process. I've also added something similar to the SDK in bug 1130529
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> I think
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Limitations_of_chrome_scripts and
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Limitations_of_frame_scripts are perfect sources of when do we need
> warnings. Also, I think we should warning for every time someone is using
> shims for various things we think are only temporary (so all the shim code
> really?)

I think this is exactly right. Hopefully the reports generated in this bug could just link to different parts of that page.
Flags: needinfo?(wmccloskey)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> - they cannot access files *warning here* what other limitations do we know?

My understanding is that we're not really enforcing this yet, for the most part, and this is almost certainly going to cause pain/breakage when we start enforcing it (or when developers trip over file access APIs that fail confusingly because they were never meant to work in that context, as in bug 1143934), notwithstanding the warnings on MDN.
Ok, I think we should agree how these warning should look like, here is my first draft.

I have yet to find an elegant way to get the file/lineno (some help would be great). I think it should be enough to link to the page itself, as it might get out of sync real quickly if we link it against its subsections, but I can be convinced otherwise. 

I think the whole stack would look too bloated, but maybe it would be useful in some cases. 

Do we want to put this behind a pref?

Anything else I'm missing?
Attachment #8583216 - Flags: feedback?(wmccloskey)
Attached image screenshot of the warning draft (obsolete) —
Comment on attachment 8583216 [details] [diff] [review]
First draft for how warnings should look like.

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

Thanks Gabor. This looks about right. One thing that would be nice is to move the reporting code to its own JSM. Eventually I'd like to do something more sophisticated than the error console, but we should start with something simple.

I do think we should like directly to subsections. If we could do it as ../Limitations_of_chrome_scripts#Whatever, and we put a warning at the top of the page to be careful about it, it should be okay.

An easier way to get the current stack is Error().stack. You don't need to throw an exception or anything. However, we also have some XPCOM APIs that seem more useful. I see nsIStackFrame, although I'm not sure how to make one. Maybe Bobby knows.

It would be really nice if the clickable location at the right side of the console linked directly to the code of the add-on. Is it possible to customize that?
Attachment #8583216 - Flags: feedback?(wmccloskey) → feedback+
Bobby, do you have any ideas about comment 11 (third paragraph)?
Flags: needinfo?(bobbyholley)
(In reply to Bill McCloskey (:billm) from comment #12)
> Bobby, do you have any ideas about comment 11 (third paragraph)?

I think Components.stack.caller should do the trick.
Flags: needinfo?(bobbyholley)
I don't want to flag this patch fir r? now, since the child side is not done yet, so it might change...

With this approach we can change the clickable link to the right to point to the JS code in the add-on that needs to be changed. Also, I use links to the sections not to the main page as requested.

The stackOffset part is not very elegant nor robust, but don't have any better idea right now and just wanted to move on for now, until we get something better. Oh, and I'm not sure if this file belongs here, or should live somewhere else.
Attachment #8583216 - Attachment is obsolete: true
Attachment #8583218 - Attachment is obsolete: true
Attachment #8585477 - Flags: feedback?(wmccloskey)
Attached patch Warnings for RemoteAddonParent (obsolete) — Splinter Review
Seems to be working nicely, I think this will be a very helpful addition. A few things: I found some undocumented shims, should we update the docs first and then finish up this patch? I've added XXX comments in the patch for those cases.

And the big question is what to do with the framescript side. If I see it correctly we have no shim layer there yet. Should we add one? I think it would be quite helpful for warnings, and also maybe for blocking a few things. As Jed pointed it out in Comment 8, there are things that should not work but still working and it's just a matter of time until we break them, so we could start throwing at this level... Anyway, do you think a shim layer is the way to go for frame scripts, or do you have a better idea? If so where should that code live and where should we hook it in? I guess the limitations are the same for process scripts as well...
Attachment #8585487 - Flags: feedback?(wmccloskey)
Comment on attachment 8585487 [details] [diff] [review]
Warnings for RemoteAddonParent

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

Thanks. This looks great!

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +294,5 @@
>  
>  ComponentRegistrarInterposition.methods.registerFactory =
>    function(addon, target, class_, className, contractID, factory) {
>      if (contractID && contractID.startsWith("@mozilla.org/network/protocol/about;1?")) {
> +      // XXX: should we warn here? do we need docs for this?

We do have docs on this:
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_frame_scripts#nsIAboutModule

@@ +374,5 @@
>      if (TOPIC_WHITELIST.indexOf(topic) >= 0) {
>        ObserverParent.addObserver(addon, observer, topic);
>      }
>  
> +    CompatWarning.warn(topic + " observer should be added from the child process only.",

Please use the new ES6 syntax: `${topic} observer should be added from the child process only.`

@@ +632,5 @@
>  let ContentDocShellTreeItemInterposition = new Interposition("ContentDocShellTreeItemInterposition");
>  
>  ContentDocShellTreeItemInterposition.getters.rootTreeItem =
>    function(addon, target) {
> +    // XXX: do we need warning? do we need docs?

I don't think we really need anything here. If they somehow got their hands on a content docshell, they must have touched one of the other shims first and gotten a warning there. Please add a comment saying that.

@@ +784,5 @@
>  let RemoteBrowserElementInterposition = new Interposition("RemoteBrowserElementInterposition",
>                                                            EventTargetInterposition);
>  
>  RemoteBrowserElementInterposition.getters.docShell = function(addon, target) {
> +  // XXX: do we need warning? do we need docs?

I added some text to https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_chrome_scripts#gBrowser.contentWindow.2C_window.content... that discusses the docShell.

@@ +821,5 @@
>    // If we don't have a CPOW yet, just return something we can use for
>    // setting the location. This is useful for tests that create a tab
>    // and immediately set contentWindow.location.
>    if (!target.contentWindowAsCPOW) {
> +    // XXX: do we need mentioning this in docs?

I added some docs about this to https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_chrome_scripts#gBrowser.contentWindow.2C_window.content.... It might be a good idea to warn additionally here since it's such a strange and unexpected situation.
Attachment #8585487 - Flags: feedback?(wmccloskey) → feedback+
Comment on attachment 8585477 [details] [diff] [review]
JSM for warnings in the shim layer

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

::: toolkit/components/addoncompat/CompatWarning.jsm
@@ +14,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +                                  "resource://gre/modules/devtools/Console.jsm");
> +
> +let CompatWarning = {
> +  warn: function CompatWarning_warn(msg, url, stackOffset)

In ES6 you can use a default param value: (msg, url, stackOffset = 3)

@@ +15,5 @@
> +                                  "resource://gre/modules/devtools/Console.jsm");
> +
> +let CompatWarning = {
> +  warn: function CompatWarning_warn(msg, url, stackOffset)
> +  {

The brace should go on the previous line.

@@ +18,5 @@
> +  warn: function CompatWarning_warn(msg, url, stackOffset)
> +  {
> +    stackOffset = stackOffset ? stackOffset : 3;
> +    let stack = Components.stack;
> +    for (let i = stackOffset; stack && i > 0; i--)

Would it work to skip stack frames until we don't see anything whose filename includes RemoteAddonsParent or CompatWarning? It's a little hacky, but I think it might be better than a fixed offset per caller.

@@ +27,5 @@
> +      // Too late during shutdown to use the nsIConsole
> +      return;
> +    }
> +
> +    let message = "Warning: " + msg + "\nMore info at: " + url;

More `...` fodder.

@@ +34,5 @@
> +               /*sourceName*/ stack ? stack.filename : "",
> +               /*sourceLine*/ stack ? stack.sourceLine : "",
> +               /*lineNumber*/ stack ? stack.lineNumber : 0,
> +               /*columnNumber*/ 0,
> +               /*flags*/ Ci.nsIScriptError.errorFlag,

Maybe this should be a warning instead of an error.

Also, at least at first, I think we should put this behind a pref. Later we might decide to turn it on for everyone, but I'd rather not do that right away.

@@ +41,5 @@
> +  },
> +
> +  chromeScriptSections: ((baseURL) => {
> +    return {
> +      content:                baseURL + "#gBrowser.contentWindow.2C_window.content...",

I'm not a big fan of this sort of indentation. Can you please remove it?
Attachment #8585477 - Flags: feedback?(wmccloskey) → feedback+
Attachment #8585477 - Attachment is obsolete: true
Attachment #8586099 - Flags: review?(wmccloskey)
Attachment #8585487 - Attachment is obsolete: true
Attachment #8586100 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #17)
> Also, at least at first, I think we should put this behind a pref. Later we
> might decide to turn it on for everyone, but I'd rather not do that right
> away.

I agree. I think we could land this part until we figure out what to do about frame/process scripts. Would it have a huge overhead to add shims to these scripts? They would do nothing by default, and when the pref is turned on then it would be used only for warning. I'm a bit afraid for the performance hit this might cause. Also I would add these shims only if the frame/process scripts are created from add-on code.
Comment on attachment 8586099 [details] [diff] [review]
part1: JSM for warnings in the shim layer

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

::: toolkit/components/addoncompat/CompatWarning.jsm
@@ +14,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +                                  "resource://gre/modules/devtools/Console.jsm");
> +
> +let CompatWarning = {
> +  warn: function CompatWarning_warn(msg, url) {

Naming this CompatWarning_warn shouldn't be necessary now. The JS engine is pretty good at inferring these names.

@@ +18,5 @@
> +  warn: function CompatWarning_warn(msg, url) {
> +    function isEnabled(){
> +      let enabled;
> +      try {
> +        enabled = Services.prefs.getBoolPref('dom.ipc.shims.enableWarnings');

Please use double quotes for string literals for consistency. Also, it's easier to use Preferences.jsm than Services.prefs: you can provide a default value and don't need the try/catch. Just do Preferences.get("dom.ipc.shims.enabledWarnings", false).

Also, could you please add dom.ipc.shims.enabledWarnings to browser/app/profile/firefox.js set to false? That way people can search for it in about:config rather than having to add a new pref.
Attachment #8586099 - Flags: review?(wmccloskey) → review+
Comment on attachment 8586100 [details] [diff] [review]
part2: Warnings for RemoteAddonParent

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

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +781,5 @@
>  let RemoteBrowserElementInterposition = new Interposition("RemoteBrowserElementInterposition",
>                                                            EventTargetInterposition);
>  
>  RemoteBrowserElementInterposition.getters.docShell = function(addon, target) {
> +  CompatWarning.warn("Direct access to content objects will no longer work in the chrome process.",

Maybe specifically mention docshells here?
Attachment #8586100 - Flags: review?(wmccloskey) → review+
I don't think we need these warnings for frame scripts. The shims don't apply there, and I don't think any of them would make sense there. Maybe you're thinking about how we could enforce rules like https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_frame_scripts ? I guess we could use the code in bug 997325 for that.
(In reply to Bill McCloskey (:billm) from comment #23)
> you're thinking about how we could enforce rules like
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Limitations_of_frame_scripts ? I guess we could use the code in bug 997325
> for that.

Yeah, I'm talking about those. Doing the warnings from C++ is a bit... heavyweight. But maybe it is the right thing to do. So let's see, we have an incomplete and growing list of interfaces one should never use from the child process. I can just add main_process_scriptable_only flag to the interfaces mentioned in the docs as a start, and pref out by default this warning as well... how about adding a link to the Limitations_of_frame_scripts? I would not bother about the sections for the interfaces from C++...

I could just handle nsIAboutModule fine from RemoteAddonsChild.jsm I guess.

And there is one more: observers other than the ones listed here: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_frame_scripts#Observers_in_the_content_process
Dave, where should we write about dom.ipc.shims.enabledWarnings flag? I want add-on developers to know about this flag (helps them with the e10s conversion), and also add-on reviewers (they should not r+ add-ons with these warnings).
Flags: needinfo?(dtownsend)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #25)
> Dave, where should we write about dom.ipc.shims.enabledWarnings flag? I want
> add-on developers to know about this flag (helps them with the e10s
> conversion), and also add-on reviewers (they should not r+ add-ons with
> these warnings).

Somewhere from here would make sense: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox
Maybe also a post on the add-ons blog. Talk to jorge/kris about telling the AMO reviewers about it.
Flags: needinfo?(dtownsend)
> I can just add main_process_scriptable_only flag to the interfaces mentioned in the docs as a
> start, and pref out by default this warning as well... how about adding a link to the
> Limitations_of_frame_scripts? I would not bother about the sections for the interfaces from
> C++...

I don't think we need a pref for that--we should always warn. The difference is that not many add-ons are using frame scripts right now, so warnings there shouldn't be too overwhelming. Let's make that a separate bug though. I'm excited for this one to land.

> I could just handle nsIAboutModule fine from RemoteAddonsChild.jsm I guess.

Not sure what you mean. However, I just noticed that your patch doesn't cover nsIAboutModule. I think we just need a warning here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm#292

It's actually confusing that the nsIAboutModule section of the documentation is under Limitations_of_frame_scripts. The issue is actually related to both chrome and frame scripts; I copied the text so it's in both now.

> And there is one more: observers other than the ones listed here: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/#Observers_in_the_content_process

Yeah, I'm not sure how to handle that. We actually have some pretty crazy code here that forbids you from observing http-on-* in the child process:
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverService.cpp#265

Maybe we could add some code that generalizes that? It should be a separate bug though.
(In reply to Bill McCloskey (:billm) from comment #27)
> there shouldn't be too overwhelming. Let's make that a separate bug though.

Cool, Bug 1150395.

> 
> > I could just handle nsIAboutModule fine from RemoteAddonsChild.jsm I guess.
> 
> Not sure what you mean. However, I just noticed that your patch doesn't
> cover nsIAboutModule. I think we just need a warning here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/
> RemoteAddonsParent.jsm#292

Actually I was quite hesitant with this warning. Since calling registerFactory
from the parent is perfectly valid, and doing so from the child is not always
necessary even. Plus even after one calls it from both side as the doc says,
the warning will not be gone... My intention was to do this in the child side
patch, when the related helper class is used, but I realized that it would not
help. Anyway, I added the warning with a note in it, that if one already does
what we suggest, then the warning should be just ignored.

> Yeah, I'm not sure how to handle that. We actually have some pretty crazy
> code here that forbids you from observing http-on-* in the child process:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverService.
> cpp#265
> 
> Maybe we could add some code that generalizes that? It should be a separate
> bug though.

I think this can go into Bug 1150395 as well.
https://hg.mozilla.org/mozilla-central/rev/4ba7172370f7
https://hg.mozilla.org/mozilla-central/rev/17068af9fd15
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: