Closed Bug 1310845 Opened 8 years ago Closed 8 years ago

Remove support for mozapp iframes

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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.
Attachment #8801950 - Flags: review?(mcmanus)
Attachment #8801950 - Flags: review?(jryans)
Attachment #8801950 - Flags: review?(fabrice)
Attachment #8801950 - Flags: review?(amarchesini)
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.  :-)
Attachment #8801950 - Flags: review?(fabrice) → review+
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/*
Attachment #8801950 - Flags: review?(mcmanus) → review+
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.
Attachment #8801950 - Flags: review?(jryans) → review+
(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...
Flags: needinfo?(ehsan)
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
Attachment #8801950 - Flags: review?(amarchesini) → review+
See Also: → 1317711
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')
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0debd7a23480
Remove support for mozapp iframes; r=fabrice,jryans,baku,mcmanus
https://hg.mozilla.org/mozilla-central/rev/0debd7a23480
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No longer blocks: 1369194
Adding ddn, just to check the docs for this.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: