|
|
Created:
6 years, 5 months ago by Matt Giuca Modified:
6 years, 5 months ago CC:
chromium-reviews, tfarina, chromium-apps-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org, Jeffrey Yasskin, scheib Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAppWindowContents: Clean up unnecessary SuspendRenderViewHost.
This is no longer necessary since crbug.com/123007 was fixed.
BUG=123007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284866
Patch Set 1 #Patch Set 2 : LoadContents doesn't need a creator_process_id. #Patch Set 3 : Remove unused includes. #Patch Set 4 : Remove more unused includes. #Patch Set 5 : Remove even more unused includes. #
Total comments: 9
Patch Set 6 : Small nits. #Patch Set 7 : Revert initial suspension of resource requests on app window open. #Messages
Total messages: 20 (0 generated)
creis: Did the groundwork for this CL. kalman: OWNERS. Hey guys, so full disclosure: I don't understand this code I'm deleting. I don't really know what an RVH is or why it was necessary to suspend it. But I'm working in this area and I found Jeremy's comments about no longer needing to do this once crbug.com/123007 is resolved, and lo, it is resolved. So I tried deleting the code and apps still seem to work just fine. Then I pulled on the thread and managed to delete a huge amount of infrastructure. So if in fact we don't need RVH suspension any more in AppWindowContents, we can save a lot of complexity. Please give careful review because I'm sure you understand this better than I do. (The important deletion is the two calls to SuspendRenderViewHost in app_window_contents.cc; everything else is just mechanically deleting stuff that is no longer needed.)
+nasko Nice cleanup, lgtm for extensions but I'm not confident to suggest you submit this now, certainly wait for creis/nasko. Perhaps you can change that check on my second comment to be a DCHECK and see what happens? I may just be entirely wrong there, and creis/nasko can point that out. https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents.cc File apps/app_window_contents.cc (left): https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents... apps/app_window_contents.cc:49: // RVH from loading anything until the background page has had a chance to RVH is the browser side of a RV. Examples of RVs include normal web pages, iframes, app windows, popups, background pages, whatever. It doesn't seem possible that the opener can be in a different process than the creator, since the only thing that can create app windows must be in an extension process (since it's a privileged function call). And all views for an extension must live in the same process. I may be misunderstanding, or wrong, so maybe this should be a [D]CHECK_EQ, though given it doesn't matter anyway, what you've done seems fine. https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/app_window/app_window_api.cc:293: created_view->GetProcess()->GetID()) nit: body should have {} since condition is 2 lines. not-nit: relating to previous comment, seems like this should be a DCHECK_EQ not an if. btw this is the same check as up on line 199, maybe that should be the same.
https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents.cc File apps/app_window_contents.cc (left): https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents... apps/app_window_contents.cc:49: // RVH from loading anything until the background page has had a chance to On 2014/07/09 13:48:50, kalman wrote: > RVH is the browser side of a RV. Examples of RVs include normal web pages, > iframes, app windows, popups, background pages, whatever. > > It doesn't seem possible that the opener can be in a different process than the > creator, since the only thing that can create app windows must be in an > extension process (since it's a privileged function call). And all views for an > extension must live in the same process. > > I may be misunderstanding, or wrong, so maybe this should be a [D]CHECK_EQ, > though given it doesn't matter anyway, what you've done seems fine. Actually one thing that does invalidate my claim will be service workers, which will be able to use app APIs but I'm not sure are guaranteed to be in the same process. Jeffrey/Vincent do you know?
https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents.cc File apps/app_window_contents.cc (left): https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents... apps/app_window_contents.cc:49: // RVH from loading anything until the background page has had a chance to On 2014/07/09 13:51:30, kalman wrote: > On 2014/07/09 13:48:50, kalman wrote: > > RVH is the browser side of a RV. Examples of RVs include normal web pages, > > iframes, app windows, popups, background pages, whatever. > > > > It doesn't seem possible that the opener can be in a different process than > the > > creator, since the only thing that can create app windows must be in an > > extension process (since it's a privileged function call). And all views for > an > > extension must live in the same process. > > > > I may be misunderstanding, or wrong, so maybe this should be a [D]CHECK_EQ, > > though given it doesn't matter anyway, what you've done seems fine. > > Actually one thing that does invalidate my claim will be service workers, which > will be able to use app APIs but I'm not sure are guaranteed to be in the same > process. Jeffrey/Vincent do you know? I'm not certain that the Service worker will always be in the same render processes as an extension. I'm also not clear now on what the implication here is if it isn't in the same process.
https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents.cc File apps/app_window_contents.cc (left): https://codereview.chromium.org/378193002/diff/80001/apps/app_window_contents... apps/app_window_contents.cc:49: // RVH from loading anything until the background page has had a chance to Well it seems that I'm not changing the behaviour when they aren't in the same process, only when they are (which is true in the normal app case). That's true for this change; not so sure about the removal of the NOTIFICATION_RENDER_VIEW_HOST_CHANGED handler. https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/app_window/app_window_api.cc:293: created_view->GetProcess()->GetID()) I'm not sure about the DCHECK vs if. I'll leave it up to the other reviewers to decide.
https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/app_window/app_window_api.cc:293: created_view->GetProcess()->GetID()) On 2014/07/10 03:37:06, Matt Giuca wrote: > I'm not sure about the DCHECK vs if. I'll leave it up to the other reviewers to > decide. An if or a CHECK I think --- if we just use a DCHECK then non equal IDs will change the routing, which could be a security issue, for non-trivial to understand code. I favor the CHECK, see if it works, in which case we'll have more certainty of the code behavior.
https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/app_window/app_window_api.cc:293: created_view->GetProcess()->GetID()) On 2014/07/10 16:53:11, scheib wrote: > On 2014/07/10 03:37:06, Matt Giuca wrote: > > I'm not sure about the DCHECK vs if. I'll leave it up to the other reviewers > to > > decide. > > An if or a CHECK I think --- if we just use a DCHECK then non equal IDs will > change the routing, which could be a security issue, for non-trivial to > understand code. I favor the CHECK, see if it works, in which case we'll have > more certainty of the code behavior. yeah, I think an if is fine.
nasko: Friendly ping. (creis is OOO for awhile so hopefully you can look at this.) https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/378193002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/app_window/app_window_api.cc:293: created_view->GetProcess()->GetID()) So I'll leave this as-is.
Ping: Nasko, would you be able to look at this?
I was out for a few days myself, so apologies for the delays. Since I'm lacking context on why this was needed in the first place, can you give a short explanation of why this was needed and not anymore?
Hmm, I had a look over this again in trying to explain it. I think I was over-zealous in removing the code (again, I don't really understand *what* this is for, just trying to clean up this TODO). The first call to SuspendRenderViewHost isn't mentioned by the comment, so I have no reason to think it should be removed (everything still works with it removed, but I don't know if it's still important for preventing the background page from running while the app window is initializing). So I have put that one back (unfortunately removing most of the nice cleanup from this CL). The other call to SuspentRenderViewHost appears to no longer be needed after creis@'s changes.
On 2014/07/17 03:56:35, Matt Giuca wrote: > Hmm, I had a look over this again in trying to explain it. I think I was > over-zealous in removing the code (again, I don't really understand *what* this > is for, just trying to clean up this TODO). > > The first call to SuspendRenderViewHost isn't mentioned by the comment, so I > have no reason to think it should be removed (everything still works with it > removed, but I don't know if it's still important for preventing the background > page from running while the app window is initializing). So I have put that one > back (unfortunately removing most of the nice cleanup from this CL). > > The other call to SuspentRenderViewHost appears to no longer be needed after > creis@'s changes. The current change looks fine to me, but I'm not sure if I'm missing something subtle. If this is just cleanup and not blocking anything, I would suggest waiting for creis@, who should be back next week.
> The current change looks fine to me, but I'm not sure if I'm missing something > subtle. If this is just cleanup and not blocking anything, I would suggest > waiting for creis@, who should be back next week. Acknowledged.
On 2014/07/21 00:33:19, Matt Giuca wrote: > > The current change looks fine to me, but I'm not sure if I'm missing something > > subtle. If this is just cleanup and not blocking anything, I would suggest > > waiting for creis@, who should be back next week. > > Acknowledged. Sorry for holding this up, but I'm happy to take a look now that I'm back from leave. Give me a minute to page the context back into my head, but it sounds like it'll be the right thing to do.
Ok, patch set 7 (which just removes the NotificationObserver) LGTM. I can confirm that we used to create and throw away an extra RVH for extensions before 123007 was fixed, and that we no longer do that. I don't see why patch set 6 would be correct. It sounds like the common case would be when the new view and the creator are in the same process, in which case the original code called SuspendRenderViewHost. If you want to remove that call in a followup CL, we should understand why it's safe to remove. (Also, there's presumably a Resume call somewhere to balance out the Suspend, and we would want to remove both if we went that route.) Hope that helps!
The CQ bit was checked by mgiuca@chromium.org
Thanks, Charlie! > Ok, patch set 7 (which just removes the NotificationObserver) LGTM. I can > confirm that we used to create and throw away an extra RVH for extensions before > 123007 was fixed, and that we no longer do that. Ack. > I don't see why patch set 6 would be correct. It sounds like the common case > would be when the new view and the creator are in the same process, in which > case the original code called SuspendRenderViewHost. If you want to remove that > call in a followup CL, we should understand why it's safe to remove. (Also, > there's presumably a Resume call somewhere to balance out the Suspend, and we > would want to remove both if we went that route.) Yeah, sounds like I was just overzealous in removing that which was actually necessary code. I don't think attempting to remove that will work.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/378193002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 284866 |