Filtering of swapped-out messages

[Łukasz]

Hello,

I've run an experiment today where I run content_browsertests, while logging which IPC messages are filtered out by SwappedOutMessages::CanSendWhileSwappedOut.  The results are:

• Allowed messages: 
  • ViewHostMsg_Focus
  • ViewHostMsg_RouteCloseEvent
  • ViewHostMsg_ShowWidget
• Disallowed / filtered-out messages:
• ViewHostMsg_HasTouchEventHandlers
• ViewHostMsg_HideValidationMessage
• ViewHostMsg_UnlockMouse
• ViewHostMsg_UpdateZoomLimits

I am not sure if the above is actionable somehow - can you please chime in on this?  In particular, I don't quite remember our discussion from a few weeks ago, but does the above mean that it might be safe to reland https://codereview.chromium.org/2941783002 (now?  after the branch point)?  Or - do you want to wait for a branch point and get rid of IPC filtering altogether?

-Lukasz

PS. I note that CanSendWhileSwappedOut message can also be overriden by ContentClient, but AFAIU this is only overriden by LayoutTestContentClient and not by //chrome layer.  Therefore, rerunning the experiment above with browser_tests probably doesn't make sense.

[Łukasz]

To help in understanding the results below, let me share the ad-hoc logging I've added:

~/src/chromium4/src on swapped-out-ad-hoc-experiments*

$ git diff origin/master -- content/common/swapped_out_messages.cc

diff --git a/content/common/swapped_out_messages.cc b/content/common/swapped_out_messages.cc

index 442cd3e1fe23..60a955bfdce2 100644

--- a/content/common/swapped_out_messages.cc

+++ b/content/common/swapped_out_messages.cc

@@ -9,10 +9,14 @@

 #include "content/common/input_messages.h"

 #include "content/common/view_messages.h"

 #include "content/public/common/content_client.h"

+#include "ipc/ipc_logging.h"

 

 namespace content {

 

 bool SwappedOutMessages::CanSendWhileSwappedOut(const IPC::Message* msg) {

+  std::string msg_name;

+  IPC::Logging::GetMessageText(msg->type(), &msg_name, msg, nullptr);

+

   // We filter out most IPC messages when swapped out.  However, some are

   // important (e.g., ACKs) for keeping the browser and renderer state

   // consistent in case we later return to the same renderer.

@@ -34,6 +38,10 @@ bool SwappedOutMessages::CanSendWhileSwappedOut(const IPC::Message* msg) {

     case ViewHostMsg_RouteCloseEvent::ID:

     // Send page scale factor reset notification upon cross-process navigations.

     case ViewHostMsg_PageScaleFactorChanged::ID:

+      LOG(ERROR) << "CanSendWhileSwappedOut"

+                 << "(allowed)"

+                 << "; msg->type() = " << msg->type()

+                 << "; msg_name = " << msg_name;

       return true;

     default:

       break;

@@ -41,7 +49,19 @@ bool SwappedOutMessages::CanSendWhileSwappedOut(const IPC::Message* msg) {

 

   // Check with the embedder as well.

   ContentClient* client = GetContentClient();

-  return client->CanSendWhileSwappedOut(msg);

+  bool result = client->CanSendWhileSwappedOut(msg);

+  if (result) {

+    LOG(ERROR) << "CanSendWhileSwappedOut"

+               << "(allowed-by-client)"

+               << "; msg->type() = " << msg->type()

+               << "; msg_name = " << msg_name;

+  } else {

+    LOG(ERROR) << "CanSendWhileSwappedOut"

+               << "(disallowed)"

+               << "; msg->type() = " << msg->type()

+               << "; msg_name = " << msg_name;

+  }

+  return result;

 }

[Charlie Reis]

Thanks!  I don't think we should land any of this before branch, since there's potential for more regressions whether we move unload after SetSwappedOut or remove swapped out IPC filtering entirely.

That said, I do think we should try to remove the IPC filtering after branch, given your results.  (Those IPCs don't seem necessary to filter, at first glance-- worth confirming, though.)  If that lands safely, your unload CL will be obviously correct, and we'll be able to rip out a bunch of old complexity.  I'd be really excited about that.

Charlie