I have a patch for this, doing a final round of testing on try.

This patch removes support for mozapp iframes, leaving support for mozbrowser iframes intact. Some of the code has been rewritten in order to phrase things in terms of mozbrowser only, as opposed to mozbrowser or app. In some places, code that was only useful with apps has been completely removed, so that the APIs consumed can also be removed. In some places where the notion of appId was bleeding out of this API, now we use NO_APP_ID. Other notions of appId which were restricted to this API have been removed.

I apologize for the enormous patch. It was really difficult to do this with a surgical knife as a lot of things were intertwined with each other. I used a sledgehammer instead. :-)

Comment on attachment 8801950 [details] [diff] [review] Remove support for mozapp iframes Review of attachment 8801950 [details] [diff] [review]: ----------------------------------------------------------------- thanks for this ehsan. its (otherwise) thankless work. r+ netwerk/*

Comment on attachment 8801950 [details] [diff] [review] Remove support for mozapp iframes Review of attachment 8801950 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall! I spotted a few nits, mostly text changes and possible additional things to remove. If you decide to clean up net monitor (as mentioned below) in this bug, please request another review for that portion. ::: caps/nsIPrincipal.idl @@ +315,5 @@ > [infallible] readonly attribute unsigned long privateBrowsingId; > > /** > * Returns true iff the principal is inside an isolated mozbrowser element. > + * <xul:browser> is not considered to be mozbrowser elements. Nit: "a mozbrowser element" ::: devtools/server/actors/webconsole.js @@ +614,4 @@ > // Start a network monitor in the parent process to listen to > // most requests than happen in parent > this.networkMonitor = > + new NetworkMonitorChild(null, this.parentActor.outerWindowID, Please remove the first arg from `NetworkMonitorChild` (in devtools/shared/webconsole/network-monitor.js) since it's now always null (or file a follow up to do so). There are various other usages of `appId` in that file for request filtering which seem like they could all now be removed as well. ::: docshell/base/nsDocShell.cpp @@ +14181,2 @@ > { > + *aIsMozBrowser = (mFrameType != FRAME_TYPE_REGULAR); I think `== FRAME_TYPE_BROWSER` would be more correct. @@ +14227,2 @@ > { > + *aIsInMozBrowser = (GetInheritedFrameType() != FRAME_TYPE_REGULAR); I think `== FRAME_TYPE_BROWSER` would be more correct. ::: docshell/base/nsIDocShell.idl @@ +832,5 @@ > * mozbrowser> or if the docshell is contained in an isolated <iframe > * mozbrowser>. > * > + * <xul:browser> is not considered to be a mozbrowser element. > + * <iframe mozbrowser noisolation> does not count as Nit: rewrap if you want...? @@ +837,5 @@ > * isolated since isolation is disabled. Isolation can only be disabled if > * the containing document is chrome. > * > * Our notion here of "contained in" means: Walk up the docshell hierarchy in > + * this process until we hit an <iframe mozbrowser> (or until the hierarchy Nit: extra space ::: docshell/base/nsILoadContext.idl @@ +105,5 @@ > [noscript] void SetRemoteTabs(in boolean aUseRemoteTabs); > > /** > * Returns true iff the load is occurring inside an isolated mozbrowser > + * element. <xul:browser> is not considered to be mozbrowser elements. Nit: "a mozbrowser element" ::: dom/base/nsFrameLoader.cpp @@ +3226,5 @@ > > nsresult > nsFrameLoader::GetNewTabContext(MutableTabContext* aTabContext, > nsIURI* aURI, > const nsACString& aPackageId) This should be a separate patch, but do we also want to remove the signed package feature here? I think it's B2G only...? ::: dom/base/nsIFrameLoader.idl @@ +205,5 @@ > */ > [infallible] attribute boolean visible; > > /** > * Find out whether the owner content really is a mozbrowser or app frame Comment needs updating. ::: dom/browser-element/BrowserElementParent.cpp @@ +58,1 @@ > // Copy the opener frame's parentApp attribute to the popup frame. Should parentApp be removed too? ::: dom/ipc/ContentParent.cpp @@ +569,5 @@ > "cacheservice:empty-cache", > }; > > // PreallocateAppProcess is called by the PreallocatedProcessManager. > // ContentParent then takes this process back within This sentence is missing an ending. @@ +574,2 @@ > /*static*/ already_AddRefed<ContentParent> > ContentParent::PreallocateAppProcess() I guess this can be removed as well, but possibly it's a separate patch. @@ +706,4 @@ > uint32_t currIdx = startIdx; > do { > + RefPtr<ContentParent> p = (*sBrowserContentParents)[currIdx]; > + NS_ASSERTION(p->IsAlive(), "Non-alive contentparent in sBrowserContntParents?"); Nit: Contnt -> Content ::: dom/ipc/ContentParent.h @@ +629,1 @@ > // Transform a pre-allocated app process into a browser process. If this "pre-allocated app process" seems like an odd term when the apps are gone. "pre-allocated content process" perhaps? ::: dom/ipc/PContent.ipdl @@ +395,5 @@ > // When the parent constructs a PBrowser, the child trusts the app token and > // other attributes it receives from the parent. In that case, the > // context should be FrameIPCTabContext. > // > // When the child constructs a PBrowser, the parent doesn't trust the app Lots of references to "app" in these comments... ::: dom/ipc/TabContext.h @@ +42,5 @@ > > /** > * Does this TabContext correspond to a mozbrowser? > * > + * <iframe mozbrowser> and <xul:browser> are not considered to be Deleting `mozapp` here makes this incorrect, since "<iframe mozbrowser>" _is_ a mozbrowser. Perhaps something like "<iframe mozbrowser> is a mozbrowser element, but <xul:browser> is not." @@ +50,5 @@ > > /** > * Does this TabContext correspond to an isolated mozbrowser? > * > + * <iframe mozbrowser> and <xul:browser> are not considered to be Needs comment cleanup here as well. @@ +141,5 @@ > > /** > * Whether this TabContext corresponds to a mozbrowser. > * > + * <iframe mozbrowser> and <xul:browser> are not considered to be Needs comment cleanup here as well. ::: modules/libpref/init/all.js @@ +964,5 @@ > pref("devtools.debugger.prompt-connection", true); > // Block tools from seeing / interacting with certified apps > pref("devtools.debugger.forbid-certified-apps", true); > // List of permissions that a sideloaded app can't ask for > +pref("devtools.apps.forbidden-permissions", ""); This pref was only used by the webapps actor, which has already been removed, so it seems safe to remove as well.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > ::: devtools/server/actors/webconsole.js > @@ +614,4 @@ > > // Start a network monitor in the parent process to listen to > > // most requests than happen in parent > > this.networkMonitor = > > + new NetworkMonitorChild(null, this.parentActor.outerWindowID, > > Please remove the first arg from `NetworkMonitorChild` (in > devtools/shared/webconsole/network-monitor.js) since it's now always null > (or file a follow up to do so). Right! > There are various other usages of `appId` > in that file for request filtering which seem like they could all now be > removed as well. > > ::: dom/base/nsFrameLoader.cpp > @@ +3226,5 @@ > > > > nsresult > > nsFrameLoader::GetNewTabContext(MutableTabContext* aTabContext, > > nsIURI* aURI, > > const nsACString& aPackageId) > > This should be a separate patch, but do we also want to remove the signed > package feature here? I think it's B2G only...? That's bug 1307456, I think. > ::: dom/browser-element/BrowserElementParent.cpp > @@ +58,1 @@ > > // Copy the opener frame's parentApp attribute to the popup frame. > > Should parentApp be removed too? Yes, good catch! > @@ +574,2 @@ > > /*static*/ already_AddRefed<ContentParent> > > ContentParent::PreallocateAppProcess() > > I guess this can be removed as well, but possibly it's a separate patch. Yes. Filed bug 1311149.

Sadly I didn't get the review for landing this in time before leaving on vacation. I need to look at this again (and deal with the bitrot) at some point when I get back...

Comment on attachment 8801950 [details] [diff] [review] Remove support for mozapp iframes Review of attachment 8801950 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +3498,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsDocShell::GetSameTypeRootTreeItemIgnoreBrowserBoundaries(nsIDocShell ** aRootTreeItem) Just because you are touching this, nsIDocShell** aRootTreeItem

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1f7dd996f7 Remove support for mozapp iframes; r=fabrice,jryans,baku,mcmanus

backed out for causing merge conflicts on m-i to m-c merge and to unblock this i had to back this out warning: conflicts while merging dom/ipc/ContentBridgeChild.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/ipc/ContentBridgeChild.h! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/ipc/ContentChild.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/ipc/ContentChild.h! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/ipc/ContentParent.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/ipc/ContentParent.h! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/ipc/TabChild.cpp! (edit, then use 'hg resolve --mark')

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bfd79155fff Backed out changeset 7d1f7dd996f7

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0debd7a23480 Remove support for mozapp iframes; r=fabrice,jryans,baku,mcmanus

Adding ddn, just to check the docs for this.

I have removed all mention of Firefox OS and mozapp from relevant pages: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement (and all related property/method/event pages) https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Chrome/API/Browser_API https://developer.mozilla.org/en-US/docs/Web/API/Using_the_Browser_API Last of all, I've added a note to the Fx53 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/53#HTMLXML Hope this is OK!