Closed Bug 1108827 Opened 5 years ago Closed 3 years ago

[e10s] Need an add-on shim for http-on-* observers

Categories

(Firefox :: Extension Compatibility, defect)

34 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
e10s + ---

People

(Reporter: billm, Assigned: billm)

References

Details

(Keywords: addon-compat, Whiteboard: shim)

Attachments

(2 files, 1 obsolete file)

I'm filing this because it came up in bug 1008768 comment 64. Here's an email that Jason wrote about this after we talked in June:

>>>
So :billm is working on making addons "just work" as-is for desktop e10s, and one issue is things like "on-modify-request", etc.  In single process Firefox addons can do things like take the channel callbacks during OMR and QI them to windows, docshells, and other lovely things that are not actually available in the parent process in e10s.   After chatting with Bill for a while, it looks like our architecture here will be something where we 1) detect magically that an addon is the thing that's getting an OMR notification, and 2) for addons, replacing the parent nsHttpChannel's callbacks from being the usual HttpChannelParentListener, and instead insert a CPOW object (Cross-Process Object Wrapper) that can forward all possible objects in the callback to the child as needed (using synchronous IPDL calls from parent->child).  That's gross (it blocks the parent process the whole time the CPOW runs) but gets us full compatibility (hopefully).

I don't think Bill has bugs open on this yet, but I wanted to give y'all a heads up that it's happening, so you can jump in with thoughts and provide help for Bill if/when he has questions.

Note that we considered an alternate architecture, where we ran things like OMR in the child (sort of: there'd still be a lot of CPOW-like stuff going on, so it's not apparently any more efficient), but it seems better to hang all this stuff off the parent's events, since there are fewer compatibility issues (the main one I can think of is that by the time on-examine-response is called in the child, we've already set cookies in the parent based on Set-Cookie headers in the reply, while if we call it in the parent the addon can remove the header before that and the cookie won't be set.  Of course we could fix that with a few more IPDL round trips if we really need to.)
<<<

We'll need some Necko support here to get the CPOW around the docshell, but I think we should be able to do that without too much trouble. I'll file a separate bug for that once this is further along.
Attached patch remote-notification-callbacks (obsolete) — Splinter Review
Hi Jason. Would you be able to provide some feedback on this patch? I'm mostly interested in where some of this code should go. I thought of trying to attach it to SerializedLoadContext somehow, but it seemed like that might be too general a place, especially for attaching the load group notification callbacks.

We'll also need some way to selectively enable this stuff. I'm not sure where the state for that should go either.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8553485 - Flags: feedback?(jduell.mcbugs)
I ended up changing this to expose the loadGroup as a CPOW rather than just loadGroup.notificationCallbacks. I don't think we have load groups on these channels in the parent, so I think it should be fine. It's a little cleaner this way.
Attachment #8553485 - Attachment is obsolete: true
Attachment #8553485 - Flags: feedback?(jduell.mcbugs)
Attachment #8554091 - Flags: feedback?(jduell.mcbugs)
Attached patch channel-shimsSplinter Review
Here is what the shims would look like, roughly.
Since this just came up in a discussion on IRC, what's the "correct", non-shimmed way to match a request to a dom window in e10s?

I've figured out that most channels have a channel.loadInfo.innerWindowID, which (hopefully) matches with the inner window IDs in child processes. But how do I find the correct browser / message manager corresponding to that ID?
You can get the topFrameElement from the load context (which you get by QIing the notificationCallbacks to nsILoadContext). The topFrameElement will be the <browser> element, so you can get its messageManager property.

Eventually, we may attach the topFrameElement to the loadInfo as well. That would make it a little easier.
Thanks for this info Bill. I've added a note to https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_chrome_scripts#HTTP_requests. When you get a moment I'd appreciate a technical review (I have tested it and it seems to work).
Thanks, that seems useful.
Duplicate of this bug: 1113294
Not sure how to get this higher on your list Jason. It's been over a month.
Flags: needinfo?(jduell.mcbugs)
the nsILoadContext workaround is all well and good for getting the top frame, but is there a way to get the actual frame associated with the request?
Why do you need the actual frame? We might be able to add something to help with that, but it would depend on the use case.
Flags: needinfo?(drew)
I'm trying to replicate the old functionality, which took the DOMWindow directly from the getInterface(Components.interfaces.nsIDOMWindow) call and changed its document.location.href.
Flags: needinfo?(drew)
Would it be possible to QI the channel to nsIHttpChannel and then call redirectTo on it? That's a little different than what you were doing, but maybe it's enough.
(In reply to Bill McCloskey (:billm) from comment #13)
> Would it be possible to QI the channel to nsIHttpChannel and then call
> redirectTo on it? That's a little different than what you were doing, but
> maybe it's enough.

Good idea, but it appears redirectTo() fails with a security error due to different domains.
(In reply to Andrew Zitnay from comment #10)
> the nsILoadContext workaround is all well and good for getting the top
> frame, but is there a way to get the actual frame associated with the
> request?

this might work:
1. (content) keep track of innerwindow IDs in content by monitoring domwindowcreated events
2. (chrome) get message manager based on topframe
3. (chrome) get innerWindowId from  .loadInfo
4. (chrome) pass down the id through the message manager
5. (content) look up dom window based on ID and act on it
Comment on attachment 8554091 [details] [diff] [review]
remote-notification-callbacks v2

Taking the review on Jason's request.
Attachment #8554091 - Flags: feedback?(jduell.mcbugs) → feedback?(honzab.moz)
Flags: needinfo?(jduell.mcbugs)
nsIHttpChannelParent.idl file is missing in the patch...
Comment on attachment 8554091 [details] [diff] [review]
remote-notification-callbacks v2

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

Who ever suggested to use CPOWs here?  Who ever suggested to have something as CPOWs at all?


This is only bringing us back to the pre-e10s hell.  Single process browser suffers from main thread flooding leading to unresponsive or badly responsive UI which is reason of big frustration of our users.  We've put a lot of effort in the past to turn any APIs used on the main thread to be absolutely non-blocking or completely asynchronous.  There are still places main thread can be blocked till these days tho.

But, now we have e10s that moves most of the main thread flood to the content process where it's not that big deal having lesser impact on user experience.  Parent process' UI is now free to respond much better.

CPOWs are turning us just back again.  What has been e10s'ed is now diffusing back, just includes the IPC overhead.

If you want any support for notification callbacks and access to objects that actually live on the child, we need to find a different way.  Any getter (synchronous) API must be non-blocking - i.e. all info must be copied to the parent to access instantly w/o any IPC.  Any setter API must be fully async, if a result is expected, expect it asynchronously or simulated.

Mirroring the load group/window trees was something I was asking about from the very start of e10s development.  It never happened.  Clearly we need something to happen here now.

This needs a wider discussion.
Attachment #8554091 - Flags: feedback?(honzab.moz) → feedback-
Comment on attachment 8554091 [details] [diff] [review]
remote-notification-callbacks v2

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

> If you want any support for notification callbacks and access to objects
> that actually live on the child, we need to find a different way.

Honza, IIUC Bill is experimenting to see if we can transparently support addons without re-writing them for e10s.   I too have doubts about whether that will will perform well enough, but it might be ok for some addons.  Are you opposed to the experiment?  I don't think we'll land this if it performs terribly.

> Mirroring the load group/window trees was something I was asking about from
> the very start of e10s development.  It never happened.  Clearly we need
> something to happen here now.

I'm not opposed to mirroring child process load groups in the parent if we need to (though it could get racy trying to keep both sides up to date).  But IIUC the use case here is mostly for addons to QI the loadGroup into other things (docshell IDLs), so duplicating the loadGroup structure on the parent may not help much with that.

::: netwerk/ipc/PNecko.ipdl
@@ +34,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +struct ChannelCompatibilityData

So far we've been putting struct types in either NeckoChannelParams.ipdlh or PHttpChannelParams.h. If this fits in one of those files that would be nice, but not a big deal.
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #19)
> Comment on attachment 8554091 [details] [diff] [review]
> remote-notification-callbacks v2
> 
> Review of attachment 8554091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > If you want any support for notification callbacks and access to objects
> > that actually live on the child, we need to find a different way.
> 
> Honza, IIUC Bill is experimenting to see if we can transparently support
> addons without re-writing them for e10s.   I too have doubts about whether
> that will will perform well enough, but it might be ok for some addons.  Are
> you opposed to the experiment?  I don't think we'll land this if it performs
> terribly.

If this is only an experiment, then we can try.  Browser hang is then our friend - if it only were reliable enough :(

Anyway, anything that has a chance to block the parent's main thread is from its nature bad.  So despite our experiment may go well for a time, I believe sooner or later it still become a bottleneck and a foot-gun.

> 
> > Mirroring the load group/window trees was something I was asking about from
> > the very start of e10s development.  It never happened.  Clearly we need
> > something to happen here now.
> 
> I'm not opposed to mirroring child process load groups in the parent if we
> need to (though it could get racy trying to keep both sides up to date). 
> But IIUC the use case here is mostly for addons to QI the loadGroup into
> other things (docshell IDLs), so duplicating the loadGroup structure on the
> parent may not help much with that.
> 

I would actually look at what all docshell provides in it's GetInterface.  But I'm afraid there is a tun of stuff we should somehow support.  This could be done incrementally - add-on devs should report bugs "this is missing and there is no way around" - and then we should add the support.  Problem is that callbacks are too generic and addon devs got used to it too much.

I would hope for some discussion widely around the whole platform, but honestly, not sure where to start right now :)  But you must admit, this topic raises more and more often lately.
> add-on devs should report bugs "this is missing and there is no way around" - and then we should add the support.

In some cases it may be sufficient to provide an easy way to locate the object in the child process. CPOWs or mirroring the structure in chrome might not be necessary.

In cases where an http-on-modify-request observer wants to modify a request based on content I think the following would be possible:

- suspend request, store it in some Map()
- find corresponding object in content, examine it
- send message back to parent
- retrieve request from map, resume it

That only hinges on the objects in question being locatable, not necessarily being available as CPOWs. Instead of blocking the entire chrome process it would just stall one request.
I want to make very clear here that the code in this bug is intended only to make legacy add-ons that have not been updated for e10s work. We also want to provide APIs for e10s that do not block the main process, but that's not what this bug is for. Once an add-on declares in its manifest that it is multiprocessCompatible, it won't have access to the shims here. We could also forbid it form using remoteNotificationCallbacks and remoteLoadGroup.

> In some cases it may be sufficient to provide an easy way to locate the object in the child
> process. CPOWs or mirroring the structure in chrome might not be necessary.

Perhaps this would be possible, but it still requires the add-on developer to change their code. The purpose of this bug is to avoid that. We should probably open a new bug to talk about APIs for when topFrameElement is insufficient.
Comment on attachment 8554091 [details] [diff] [review]
remote-notification-callbacks v2

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

::: netwerk/protocol/http/HttpChannelParent.h
@@ +180,5 @@
>    PBOverrideStatus mPBOverride;
>  
>    nsCOMPtr<nsILoadContext> mLoadContext;
> +  nsCOMPtr<nsISupports>    mRemoteNotificationCallbacks;
> +  nsCOMPtr<nsISupports>    mRemoteLoadGroup;

> We could also forbid it form using remoteNotificationCallbacks and remoteLoadGroup.

Yes, and let's at least put some scary comment in the C++ so no one uses these accidentally for other purposes:

   // remote access to callbacks/loadgroup. For legacy addon support only.
Comment on attachment 8554091 [details] [diff] [review]
remote-notification-callbacks v2

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

based on further discussion this patch is OK for me

please add nsIHttpChannelParent.idl

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1094,5 @@
> +  ContentChild* cc = ContentChild::GetSingleton();
> +  if (jsipc::CPOWManager* cpowManager = cc->GetCPOWManager()) {
> +    nsCOMPtr<nsIInterfaceRequestor> nc;
> +    GetNotificationCallbacks(getter_AddRefs(nc));
> +    nsresult rv = cpowManager->WrapObject(nc, &aCompatData->notificationCallbacks());

not sure where this is declared/defined.  but if this works, then the patch looks good to me.
Attachment #8554091 - Flags: feedback- → feedback+
Someone in bug 1143006 suggested it may dupe this.

I'm very unclear whether that is true.  In my case the code is already in the child process.  Greasemonkey is trying to do:

( https://github.com/greasemonkey/greasemonkey/blob/16fb34c8f32f13272ca6bd744d3284d5cc4a7744/modules/xmlhttprequester.js#L152 )

    var channel = subject.QueryInterface(Components.interfaces.nsIChannel);
    if (channel == req.channel) {
      var httpChannel = subject.QueryInterface(Components.interfaces.nsIHttpChannel);
      httpChannel.setRequestHeader("Referer", details.headers.Referer, false);
    }
    
Inside a "http-on-modify-request" observer, but it is the `.addObserver()` which fails, again in the child process.  Should I expect this to work in the future, or should I be trying to change my code to address this?
There's no reason addObserver should be failing in the content process. It's worth figuring out what exception is being thrown and filing a separate bug for that.
Thanks, that's the mentioned bug 1143006 then.
(In reply to The 8472 from comment #15)
> (In reply to Andrew Zitnay from comment #10)
> > the nsILoadContext workaround is all well and good for getting the top
> > frame, but is there a way to get the actual frame associated with the
> > request?
> 
> this might work:
> 1. (content) keep track of innerwindow IDs in content by monitoring
> domwindowcreated events
> 2. (chrome) get message manager based on topframe
> 3. (chrome) get innerWindowId from  .loadInfo
> 4. (chrome) pass down the id through the message manager
> 5. (content) look up dom window based on ID and act on it

Do you have any sample code for step 1?  I've tried both:

window.addEventListener('DOMWindowCreated', function() { alert(1); }, false);

and:

document.addEventListener('DOMWindowCreated', function() { alert(1); }, false);

from a page-mod script, but it's not detecting inserted iframes.

I've found that I can do;

window.addEventListener('DOMNodeInserted', function(event) { if (event.node.nodeName == 'IFRAME') { alert(1); } }, false);

Is there any way to get the inner window ID from DOMNodeInserted?
(In reply to Andrew Zitnay from comment #28)
> from a page-mod script, but it's not detecting inserted iframes.

The APIs I was referring to a privileged and probably not available in the page-mod sandbox. You'll need a framescript for that

see https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox and subpages for further information
(In reply to The 8472 from comment #29)
> (In reply to Andrew Zitnay from comment #28)
> > from a page-mod script, but it's not detecting inserted iframes.
> 
> The APIs I was referring to a privileged and probably not available in the
> page-mod sandbox. You'll need a framescript for that
> 
> see https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox and
> subpages for further information

Thanks, I think I'm well on my way to a solution.
(In reply to Andrew Zitnay from comment #30)

> Thanks, I think I'm well on my way to a solution.

Please don't listen for DOMNodeInserted events, though. 
This is deprecated and causes serious and persistent DOM performance issues, see https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Mutation_events#Performance

IIRC, it's also ground for AMO review rejection.
(In reply to Giorgio Maone from comment #31)
> (In reply to Andrew Zitnay from comment #30)
> 
> > Thanks, I think I'm well on my way to a solution.
> 
> Please don't listen for DOMNodeInserted events, though. 
> This is deprecated and causes serious and persistent DOM performance issues,
> see
> https://developer.mozilla.org/en-US/docs/Web/Guide/Events/
> Mutation_events#Performance
> 
> IIRC, it's also ground for AMO review rejection.

Yeah, I know.  If I needed it, I was planning on only listening right before the IFRAME was inserted, then stopping listening right after.  But DOMWindowCreated worked, so it doesn't matter.
(In reply to The 8472 from comment #15)
> this might work:
> 1. (content) keep track of innerwindow IDs in content by monitoring
> domwindowcreated events
> 2. (chrome) get message manager based on topframe
> 3. (chrome) get innerWindowId from  .loadInfo
> 4. (chrome) pass down the id through the message manager
> 5. (content) look up dom window based on ID and act on it


I suppose parent process is already keeping track of all content windows.

Couldn't this data (innerID, URL) be made available to extensions?
(In reply to Jeferson Hultmann from comment #33)
> I suppose parent process is already keeping track of all content windows.
> 
> Couldn't this data (innerID, URL) be made available to extensions?

I think it might be possible to keep the tracking part by using nsIWindowMediator.getCurrentInnerWindowWithId(id)[1]

But I guess that only works in the content process, so you would still have to do the message passing. Why do you need it in the parent?





[1] http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsIWindowMediator.idl#79
(In reply to The 8472 from comment #34)
> But I guess that only works in the content process, so you would still have
> to do the message passing. Why do you need it in the parent?


I need to know the URL of the associated window of a request/response.
(In reply to Jeferson Hultmann from comment #35)
> I need to know the URL of the associated window of a request/response.

Which uri precisely? I think you can already get a bunch of uris from the request:

a) the tab url via .currentURI from the <browser> element. see comment 5
b) you can get request.loadInfo.triggeringPrincipal.URI which more or less reflects the current iframe/nested document if I understand correctly
c) you can get request.loadInfo.loadPrincipal.URI which takes concerns like cross-origin stylesheets loading images into account, i.e. gives a better view of the dependency chain of origins
d) the target URI of the request (obviously)
(In reply to The 8472 from comment #36)

> b) you can get request.loadInfo.triggeringPrincipal.URI which more or less
> reflects the current iframe/nested document if I understand correctly


Thanks, this one seems to be what I need.

Also I need the location of all the parent windows. Is there a way to do it?
You'll (In reply to Jeferson Hultmann from comment #37)
> Also I need the location of all the parent windows. Is there a way to do it?

You didn't mention that before, at all. If you need the whole hierarchy you'll have to talk to the child process.
(In reply to The 8472 from comment #38)
> You didn't mention that before, at all. If you need the whole hierarchy
> you'll have to talk to the child process.

filed bug 1167989
triage team - this bug is also here to understand if this has been handled in another manner.

i would believe it's a "won't fix", as we're not making more shims.
Whiteboard: shim
For the record, it looks like e10s compat for 1Password is blocked on this ( https://discussions.agilebits.com/discussion/comment/321193/#Comment_321193 )
hi bill, just wondering if you are planning to land this shim patch or if it was done in a different way.  seems like you were just needing final review.
Flags: needinfo?(wmccloskey)
I'm not planning to land this. It just causes us to use more CPOWs, which leads to really slow add-ons. If 1Password has specific needs, they should file a bug with details of what they need.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(wmccloskey)
Resolution: --- → WONTFIX
So if this was closed wontfix, is there any issue tracking an alternative for getting the nsIHttpChannel from http-on-examine-response event? If I understand correctly, event.subject.QueryInterface(Ci.nsIHttpChannel) is getting deprecated?
My understanding is that if your observer is in the parent process you can access the channel just fine, just not the associated content DOM (without using asynchronous messaging APIs).
You need to log in before you can comment on or make changes to this bug.