Closed Bug 1012639 Opened 7 years ago Closed 7 years ago

IsSameProcess returns false-negatives with OMTC enabled

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Fix IsSameProcess (obsolete) — Splinter Review
IsSameProcess doesn't actually check to see if the returned process handle matches the current process (it just checks if it's invalid). This causes false-negatives when OMTC is enabled.
Attachment #8424793 - Flags: review?(bjacob)
Attachment #8424793 - Flags: review?(bjacob) → review+
Comment on attachment 8424793 [details] [diff] [review]
Fix IsSameProcess

Review of attachment 8424793 [details] [diff] [review]:
-----------------------------------------------------------------

You need someone who really knows our IPC system to review this patch. The current IsSameProcess implementation was based on the assumption that the IPC system will set mOtherProcess to kInvalidProcessHandle in the same-process case i.e. when there is no "other process". Was that assumption wrong?
Attachment #8424793 - Flags: review+ → review?(bent.mozilla)
Note that this assumption was the intent of bug 974356 patch 1 (attachment 8381042 [details] [diff] [review]). Was that patch not working as intented?
(In reply to Benoit Jacob [:bjacob] from comment #1)
> Comment on attachment 8424793 [details] [diff] [review]
> Fix IsSameProcess
> 
> Review of attachment 8424793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need someone who really knows our IPC system to review this patch. The
> current IsSameProcess implementation was based on the assumption that the
> IPC system will set mOtherProcess to kInvalidProcessHandle in the
> same-process case i.e. when there is no "other process". Was that assumption
> wrong?

This assumption does appear to be wrong - By observation, this is the case in LayerTransactionParent, but not the case in ShadowLayerForwarder, where OtherProcess and GetCurrentProcessHandle are the same in non-remote browser frames with OMTC enabled.
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Note that this assumption was the intent of bug 974356 patch 1 (attachment
> 8381042 [details] [diff]). Was that patch not working as intented?

I'm not authorised to see this bug or patch.
Sorry; CC'd you.
Ben: is the right approach here to fix mOtherProcess so that it really always has the value kInvalidProcessHandle in the same-process case, or is it instead to stop making this assumption? In the latter case, please review Chris' patch?
Flags: needinfo?(bent.mozilla)
Comment on attachment 8424793 [details] [diff] [review]
Fix IsSameProcess

Yeah, this would work, but I'd rather remove the footgun by ensuring that mOtherProcess is always kInvalid when we're dealing with same-process actors.
Attachment #8424793 - Flags: review?(bent.mozilla) → review-
Flags: needinfo?(bent.mozilla)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #7)
> Comment on attachment 8424793 [details] [diff] [review]
> Fix IsSameProcess
> 
> Yeah, this would work, but I'd rather remove the footgun by ensuring that
> mOtherProcess is always kInvalid when we're dealing with same-process actors.

okidokes, I'll rework the patch like that tomorrow morning.
Unless I missed something, we're still not understanding why the value of mOtherProcess is not kInvalidProcessHandle as it should be. That's what would need to be debugged here!
(In reply to Benoit Jacob [:bjacob] from comment #9)
> Unless I missed something, we're still not understanding why the value of
> mOtherProcess is not kInvalidProcessHandle as it should be. That's what
> would need to be debugged here!

I'll figure this out also :)
Here's the fix, and the reason it was happening is made obvious by the diff.

Benoit, is there someone else that can review this before Ben gets back?
Attachment #8424793 - Attachment is obsolete: true
Attachment #8426964 - Flags: review?(bent.mozilla)
Flags: needinfo?(bjacob)
Excellent, thanks! So my bug 974356 patch 1 forgot to change this case. Sorry!

As Ben reviewed it, he really should review this too :-)

But if you're in a huge rush to land this within the next few hours, you could ask any other IPDL peer, I guess. You could ask :bsmedberg.
Flags: needinfo?(bjacob)
Comment on attachment 8426964 [details] [diff] [review]
Fix OtherProcess sometimes being the current process

Flagging bsmedberg for review - This (one line patch) is blocking turning OMTC on on Linux, so whoever gets to it first :)
Attachment #8426964 - Flags: review?(benjamin)
Would you mind attaching a sample of the generated code before and after this change? Thanks!
Flags: needinfo?(chrislord.net)
This is a diff of the change in generated code pre and post-patch for a particular file (the same change is reflected in several other files) - let me know if you'd like me to attach the whole files, or show the whole diff between builds (which is just this change, but in several files), but this is the only difference:

diff -u -p pre-patch/PContentParent.cpp post-patch/PContentParent.cpp
--- pre-patch/PContentParent.cpp	2014-05-28 18:09:41.265137328 +0100
+++ post-patch/PContentParent.cpp	2014-05-28 18:13:50.722300632 +0100
@@ -224,7 +224,7 @@ auto PContentParent::Open(
         MessageLoop* aMessageLoop,
         mozilla::ipc::Side aSide) -> bool
 {
-    mOtherProcess = base::GetCurrentProcessHandle();
+    mOtherProcess = ipc::kInvalidProcessHandle;
     return (mChannel).Open(aChannel, aMessageLoop, aSide);
 }
Flags: needinfo?(chrislord.net)
Comment on attachment 8426964 [details] [diff] [review]
Fix OtherProcess sometimes being the current process

Review of attachment 8426964 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it will break bug 827825 again, right?

I think we're slowly unraveling what that bug tried to do. We'll need another approach one way or another, but this patch by itself doesn't look like it will work.
Attachment #8426964 - Flags: review?(bent.mozilla)
Attachment #8426964 - Flags: review?(benjamin)
Attachment #8426964 - Flags: review-
In which case, should we do what I originally did in the obsolete patch? (you gave this an r- and told me to do this in comment #7)
Flags: needinfo?(bent.mozilla)
Well, I'll check with Bas, but it's looking more like we need to just change everything to not use kInvalidProcessHandle anymore and instead always make mOtherProcess be 'this process' for same-process actors.

To be clear I like kInvalidProcessHandle better but I wasn't aware of this problem until now...
Flags: needinfo?(bent.mozilla)
This version should not regress bug 827825 - diff vs. last patch:


diff --git a/ipc/glue/Shmem.cpp b/ipc/glue/Shmem.cpp
--- a/ipc/glue/Shmem.cpp
+++ b/ipc/glue/Shmem.cpp
@@ -618,16 +618,22 @@ Shmem::GetSysVID() const
 
 IPC::Message*
 Shmem::ShareTo(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead,
                base::ProcessHandle aProcess,
                int32_t routingId)
 {
   AssertInvariants();
 
+  // kInvalidProcessHandle for OtherProcess() is used to indicate that it's
+  // the same process.
+  if (aProcess == kInvalidProcessHandle) {
+    aProcess = base::GetCurrentProcessHandle();
+  }
+
   if (SharedMemory::TYPE_BASIC == mSegment->Type()) {
     SharedMemoryBasic* seg = static_cast<SharedMemoryBasic*>(mSegment);
     SharedMemoryBasic::Handle handle;
     if (!seg->ShareToProcess(aProcess, &handle))
       return 0;
 
     return new ShmemCreated(routingId, mId, mSize, handle);
   }
Attachment #8426964 - Attachment is obsolete: true
Attachment #8436974 - Flags: review?(bent.mozilla)
Comment on attachment 8436974 [details] [diff] [review]
Fix OtherProcess sometimes being the current process v2

Review of attachment 8436974 [details] [diff] [review]:
-----------------------------------------------------------------

I think this should be ok, thanks!

::: ipc/glue/Shmem.cpp
@@ +622,5 @@
>                 int32_t routingId)
>  {
>    AssertInvariants();
>  
> +  // kInvalidProcessHandle for OtherProcess() is used to indicate that it's

Nit: OtherProcess() isn't really relevant here, I'd leave that bit out.
Attachment #8436974 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/22cf4fda6c8d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.