Created attachment 8550814 [details] [diff] [review] top-frame-element Given an nsIChannel, it's common for add-on authors to do something like the following: var ir = channel.notificationCallbacks.QueryInterface(Ci.nsIInterfaceRequestor); var domWindow = ir.getInterface(Ci.nsIDOMWindow); This doesn't work in e10s because the content window lives in the content process. Instead we'd like them to do this: var ir = channel.notificationCallbacks.QueryInterface(Ci.nsIInterfaceRequestor); var loadContext = ir.getInterface(Ci.nsILoadContext); var browser = loadContext.topFrameElement; Most of the time they can get everything they need from the <browser> element. The problem with this change is that it only works in e10s. In non-e10s, loadContext.topFrameElement returns null. We really want it to work in both. Here's why it returns null in non-e10s: channel.notificationCallbacks is a docshell loadContext is also a docshell nsDocShell::GetTopFrameElement calls GetFrameElement on the content nsGlobalWindow GetFrameElement calls GetRealFrameElement GetRealFrameElement returns null because it won't let us cross the content/chrome boundary It seems to me that this makes nsDocShell::GetTopFrameElement pretty useless. Won't we always cross the content/chrome boundary when trying to get the top window's frame element? To fix this I wrote a patch that uses GetFrameElementInternal instead. That skips the content/chrome check. I'm worried this might break things, but it seems fairly safe. Content JS shouldn't be able to get to this docshell method. There aren't many uses of topFrameElement in the tree. I'm a bit worried about b2g, but my understanding of <iframe mozbrowser> is that it's typically used in a content window, so it shouldn't be affected by this change. Asking Jonas for feedback just in case though. Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7067550190d1
Comment on attachment 8550814 [details] [diff] [review] top-frame-element This would sort of break http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-helper.js?rev=c4d908fa4442#209 so at least the documentation there should be fixed. And we need to update the documentation in nsILoadContext.idl. I'd like to see a patch those fixed, but other than that I think we can make the change. (even better would be to change the name of the property to something which describes it better, but I don't have a good suggestion.)
Attachment #8550814 - Flags: review?(bugs) → review-
Why would it break the network-helper code? I think it would allow us to remove this code (at the only call site): https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-monitor.js#612-625 But I don't see how anything would be broken.
"sort of". It would break the API as written in the documentation. Not really break the users of the API. So just clarify the documentation.
Created attachment 8552639 [details] [diff] [review] top-frame-element 2 I updated the comments to reflect the fact that topFrameElement is now valid in e10s and non-e10s. Is that what you wanted? I also removed any suggestion that topFrameElement might be null (although to be honest I'm not sure what topFrameElement is for a top-level chrome docshell).
It is null for top level docshell.
Comment on attachment 8552639 [details] [diff] [review] top-frame-element 2 > /** > * topFrameElement is the <iframe> or <frame> or <browser> element which contains the > * topWindow with which the load is associated. > * > * Note that we may have a topFrameElement even when we don't have an > * associatedWindow, if the topFrameElement's content lives out of process. >+ * topFrameElement is available in single-process and multiprocess contexts. Please add a comment that we may cross content/chrome boundary here. > /** >- * Gets the topFrameElement that is associated with aRequest. >+ * Gets the topFrameElement that is associated with aRequest. This >+ * works in single-process and multiprocess contexts. Add a note that this may cross the content|chrome boundary. That is rather important piece of the API. > * > * @param nsIHttpChannel aRequest >- * @returns nsIDOMElement|null >- * The top frame element for the given request, if available. >+ * @returns nsIDOMElement >+ * The top frame element for the given request. keep the null
Attachment #8552639 - Flags: review?(bugs) → review+
Hi Bill, I think it's indeed fine to fix up loadContext.topFrameElement to return the same thing in non-e10s builds as we do in e10s builds. However, if we're asking developers to change their code, I'd really rather not point them at anything that uses channel.notificationCallbacks or nsILoadContext. Both > var ir = > channel.notificationCallbacks.QueryInterface(Ci.nsIInterfaceRequestor); > var domWindow = ir.getInterface(Ci.nsIDOMWindow); and > var ir = > channel.notificationCallbacks.QueryInterface(Ci.nsIInterfaceRequestor); > var loadContext = ir.getInterface(Ci.nsILoadContext); > var browser = loadContext.topFrameElement; are pretty gross since they depend on ambigious getInterface calls. I'm actually working on deprecating channel.notificationCallbacks since really the only way to use it is through ambigious getInterface calls which rely on a lot of undocumented knowledge about internals of gecko. Likewise nsILoadContext adds a bunch of extra functionality to docshell (which really has too much stuff in it as it is). Also, nsILoadContext is generally used through above mentioned ambigious getInterface calls. Instead I'd much rather add a nsILoadInfo.topFrameElement getter. That way code could simply do > var browser = channel.loadInfo.topFrameElement; It should be quite doable to make that work in both e10s and non-e10s. Of course, it's totally up to Olli and bz as docshell peers/owners if we should add more stuff to docshell. But I really want to get addon authors off of using nsILoadContext and channel.notificationCallbacks.
This is not adding anything to docshell, just refining the behavior of one attribute. But ok, channel.loadInfo.topFrameElement should possibly return the same thing what loadContext.topFrameElement does.
It's good if either way we're not adding anything to docshell here. However if we're telling addon authors to change their code (which presumably we are if this code doesn't work in non-e10s builds), then I think we really should point them at something that doesn't depend on nsIChannel.notificationCallbacks or nsILoadContext. Either way we will soon be telling people to stop using both those, so it's nice if we can avoid telling people to change their code multiple times.
Why would we be telling people to stop using nsILoadContext? The whole point of nsILoadContext was to wean people off docshell and onto something more e10s-friendly, no?
(In reply to Boris Zbarsky [:bz] from comment #10) > Why would we be telling people to stop using nsILoadContext? The whole > point of nsILoadContext was to wean people off docshell and onto something > more e10s-friendly, no? Yeah, a while ago someone told me that nsILoadContext was the "new" think that we should use. I'm fine doing whatever you guys decide. However, I do think we should take this patch regardless. We already expose topFrameElement. This just makes it work properly. If we do decide that nsILoadInfo is the way to go, we should document it. Given the similarity of the names, the whole situation is very confusing.
nsILoadContext mainly suffers from that you get it through notificationsCallback which is a big ball of unhappyness. But also the fact that you get it through the loadgroup means that it breaks for sendBeacon (and possibly eventually <a ping>). It's also problematic that nsILoadInfo and nsILoadContext duplicate a lot of information. So I think we should consolidate to one or the other. It seems easier to me to consolidate the information in nsILoadInfo than to consolidate it to nsILoadContext given that nsILoadContext has to be the same instance across all loads within a document. So flags that are specific to a request doesn't really fit. My hope was that nsILoadInfo can eventually replace nsILoadContext. LoadInfo already covers much of the important information such as innerWindowID and appid/browserflag. I think all of the other information can be derived using the data that LoadInfo already has. Such as topLevelFrame and isAppOfType.
I filed bug 1124477 top add topFrameElement to nsILoadInfo. Thinking about it a little more, it is pretty annoying that you need to look at both the request and the loadGroup to get the correct nsIContextContext. I don't actually understand which one is expected to be valid at any time. I doubt most add-on authors do either. Usually this code is wrapped in a big try/catch clause, which is usually a sign that something is pretty broken somewhere. If we could fix that bug moving things to nsILoadInfo, I'd be all for it.
(In reply to Bill McCloskey (:billm) from comment #14) > I filed bug 1124477 top add topFrameElement to nsILoadInfo. Thinking about > it a little more, it is pretty annoying that you need to look at both the > request and the loadGroup to get the correct nsIContextContext. I don't > actually understand which one is expected to be valid at any time. I doubt > most add-on authors do either. Usually this code is wrapped in a big > try/catch clause, which is usually a sign that something is pretty broken > somewhere. If we could fix that bug moving things to nsILoadInfo, I'd be all > for it. Yup, this is one of the big problems with .notificationCallbacks. The intended approach is basically "try to do what you want to do using both the request's callback and the loadgroup's callback. If what you're trying to do works off of either you're probably good". I.e. the whole thing is a "miscellaneous stuff" feature which over the years has grown past what its design could handle. (Arguably the design was pretty poor even when it was added, which was way before any of the people on this bug was involved).
> So I think we should consolidate to one or the other. OK, that's fair. > given that nsILoadContext has to be the same instance across all loads within a document It totally doesn't and isn't, fwiw. But I'm fine with collapsing this stuff into loadinfo.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.