[mojo-edk] Expose notification source to MojoWatch callbacks

mojo/edk/system/request_context.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/edk/system/request_context.h"
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/threading/thread_local.h"
 
 namespace mojo {
 namespace edk {
 
 namespace {
 
 base::LazyInstance<base::ThreadLocalPointer<RequestContext>>::Leaky
     g_current_context = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
-RequestContext::RequestContext() : tls_context_(g_current_context.Pointer()){
+RequestContext::RequestContext() : RequestContext(Source::LOCAL_API_CALL) {}
+
+RequestContext::RequestContext(Source source)
+    : source_(source), tls_context_(g_current_context.Pointer()) {
   // We allow nested RequestContexts to exist as long as they aren't actually
   // used for anything.
   if (!tls_context_->Get())
     tls_context_->Set(this);
 }
 
 RequestContext::~RequestContext() {
   // NOTE: Callbacks invoked by this destructor are allowed to initiate new
   // EDK requests on this thread, so we need to reset the thread-local context
   // pointer before calling them.
+  RequestContext::Source notification_source = tls_context_->Get()->source();

[Anand Mistry (off Chromium), 2016/03/17 01:53:09]

I don't think this is what we want. Consider a stack with an external context
and a nested local one. When the nested local context is destroyed,
tls_context->source is external since the tls refers to the bottom most context
on the stack. So below, any notification is going to be run with from_system as
the source instead of none since this is a local context...

...but... at the same time, there shouldn't be any pending watch callbacks
because this context was nested and nested contexts should be ignored and not
have any watch callbacks.

[Ken Rockot(use gerrit already), 2016/03/17 01:56:36]

That was the intent. I thought that's what you were suggesting earlier and it
seems correct to me. What's wrong with these calls running in a system context
if they are in fact rooted there on the stack? Perhaps it's not semantically
accurate from some perspective, but that's an internal detail.

[Ken Rockot(use gerrit already), 2016/03/17 15:44:29]

I've updated the destructor.

Do you agree that all this logic is essentially correct for the case where the
dtor is for the outermost context, and that for all other cases we should just
CHECK that the finalizer list is empty?

We need a new outer context to run each of these callbacks and I can see no
reason why they shouldn't inherit Source from the one we just tore down.

   if (IsCurrent())
     tls_context_->Set(nullptr);
 
+  MojoWatchNotificationFlags flags = MOJO_WATCH_NOTIFICATION_FLAG_NONE;
+  if (notification_source == Source::SYSTEM)
+    flags |= MOJO_WATCH_NOTIFICATION_FLAG_FROM_SYSTEM;
+
   for (const WatchNotifyFinalizer& watch :
       watch_notify_finalizers_.container()) {
     // Establish a new request context for the extent of each callback to ensure
     // that they don't themselves invoke callbacks while holding a watcher lock.
-    RequestContext request_context;
-    watch.watcher->MaybeInvokeCallback(watch.result, watch.state);
+    RequestContext request_context(notification_source);
+    watch.watcher->MaybeInvokeCallback(watch.result, watch.state, flags);
   }
 
   for (const scoped_refptr<Watcher>& watcher :
           watch_cancel_finalizers_.container())
     watcher->Cancel();
 }
 
 // static
 RequestContext* RequestContext::current() {
   DCHECK(g_current_context.Pointer()->Get());
@@ skipping 22 matching lines @@
     scoped_refptr<Watcher> watcher,
     MojoResult result,
     const HandleSignalsState& state)
     : watcher(watcher), result(result), state(state) {
 }
 
 RequestContext::WatchNotifyFinalizer::~WatchNotifyFinalizer() {}
 
 }  // namespace edk
 }  // namespace mojo