Temporarily avoid process swaps for renderer navigations away from an app.

Temporarily avoid process swaps for renderer navigations away from an app.

BUG=65953
BUG=59285
TEST=AppApiTest.AppProcess

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69338

[Charlie Reis, 2010-12-13 20:08:37]

For the sake of discussion, here's a potential workaround for the window.opener
problem affecting OAuth-style logins in new windows (http://crbug.com/65953 and
likely http://crbug.com/59285).

I'd much rather get window.opener restored if the window navigates back to the
original site, but I'm concerned that's a bigger change that will take a little
while to get fixed.  This heuristic avoids swapping out of the app process on
renderer-initiated navigations if window.opener is valid, which is a reasonable
sign that the window and its parent may communicate in the future.

Erik, how bad is it if non-app URLs load in the app's process, in terms of the
app's permissions?  Would it be better to leave this broken and keep non-app
URLs out of the process, or is this a reasonable short-term approach?

[Charlie Reis, 2010-12-14 00:40:02]

Here's a version closer to what we've been discussing.  It only swaps processes
on renderer navigations if the current URL is not already an app (to avoid
breaking script connections within an app), and if there is no window.opener (to
avoid disrupting OAuth logins).

Thoughts?

[darin (slow to review), 2010-12-14 17:17:40]

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:396: if (!RenderThread::current())
what is the purpose of this null check?  i don't see RenderThread::current()
dereferenced below.

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:402: if (old_url.is_empty() && frame->opener())
Why do you bother checking frame->opener() here?  At the callsite, you check
frame->opener() as well.  If the opener is non-null here, then the result of
InExtensionExtent will be ignored anyways.

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3019: // Temporary workaround for
crbug.com/65953: Only swap processes if we
nit: Comments like this should have a TODO(creis)

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3024: frame->opener() == NULL) {
style nit: prefer "!frame->opener()"

[Charlie Reis, 2010-12-14 18:52:29]

Here's a simpler CL that doesn't check for window.opener anymore, as we
discussed.  CrossesExtensionExtents was only used for this check, so I've
temporarily changed it into CrossesIntoExtensionExtent.

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:396: if (!RenderThread::current())
On 2010/12/14 17:17:40, darin wrote:
> what is the purpose of this null check?  i don't see RenderThread::current()
> dereferenced below.

We don't need this function anymore, but the same check is in
CrossesExtensionExtents above.  Erik, do you know why it's there?

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:402: if (old_url.is_empty() && frame->opener())
On 2010/12/14 17:17:40, darin wrote:
> Why do you bother checking frame->opener() here?  At the callsite, you check
> frame->opener() as well.  If the opener is non-null here, then the result of
> InExtensionExtent will be ignored anyways.

No longer needed here, but still used in CrossesExtensionExtents.

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3019: // Temporary workaround for
crbug.com/65953: Only swap processes if we
On 2010/12/14 17:17:40, darin wrote:
> nit: Comments like this should have a TODO(creis)

Done.

http://codereview.chromium.org/5766003/diff/5001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:3024: frame->opener() == NULL) {
On 2010/12/14 17:17:40, darin wrote:
> style nit: prefer "!frame->opener()"

Simplified by removing this part of the check and modifying
CrossesExtensionExtents instead.

[Charlie Reis, 2010-12-15 19:43:20]

I checked with Matt and it's likely the RenderThread check isn't necessary. 
(It's usually helpful for uses of the thread, which might be null during unit
tests.)

I've removed it, and we should be fine if the try jobs pass.

[darin (slow to review), 2010-12-15 20:55:19]

LGTM