Closed Bug 1046245 Opened 10 years ago Closed 9 years ago

implement navigator.mediaDevices for enumeration local gUM audio & video sources

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dmosedale, Assigned: jib)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [gUM])

Attachments

(6 files, 57 obsolete files)

19.05 KB, patch
jib
: review+
Details | Diff | Splinter Review
6.85 KB, patch
jib
: review+
Details | Diff | Splinter Review
25.00 KB, patch
jib
: review+
Details | Diff | Splinter Review
18.46 KB, patch
jib
: review+
Details | Diff | Splinter Review
60.36 KB, patch
jib
: review+
Details | Diff | Splinter Review
23.82 KB, patch
jesup
: review+
Details | Diff | Splinter Review
See also https://plus.google.com/+FrancoisBeaufort/posts/AShvJNfzsCg
and https://plus.google.com/+FrancoisBeaufort/posts/ZpMFViU3sHG

Note: we'll need to keep the WG decisions and privacy/etc concerns about availability of this information in mind
Blocks: 1011878
Blocks: 1122798
Assignee: nobody → jib
Working version testable with jsfiddle.net/21tzumsL

BTW, this exposes that our audio devices - at least on OSX - don't have ids, it's just the name.

Webidl framework is done. Code is a tad ugly. I started cleaning up a little, only realizing that I should probably clean up a lot instead (maybe replace nsIMediaDevice entirely? but it has several users), so I put it off (great logic!) Lets see if landing anything at all for 38 is realistic first.

Remaining work: anonymize the id, so that it is not a super-cookie, and remove label existing gUM permission is not detected.
Attachment #8566876 - Flags: feedback?(rjesup)
Attachment #8566876 - Flags: feedback?(bugs)
Like everything device-related, automating tests for this will be hard. Wrinkle: No way to pass fake:true to enumerateDevices() :-/ I'll add a test that exercises the code at least.
On a separate bug, someone suggested that we provide a pref to force the use of fake devices.  It follows that the set of devices would be reduced to a single fake device.

As for the supercookie, I don't know where to persist site-specific data, but all you need is some random key (one that shares fate with cookies on that origin) that you use with HMAC to generate a new ID.

I was going to show you how to do that using WebCrypto, but it looks completely unusable, so I won't.
A pref, that makes sense. Yeah a single fake device isn't all that exciting is it.

Thanks for chiming in on the supercookie, you'll know if this is unsound: My naive spitball idea so far was to store a random value per origin, and use that value as a PRNG seed. E.g. the first 20 values would be the keys for the 20 first devices, e.g. index 0-19 into a globally kept list of devices seen.
I wouldn't use the PRNG, though a good secure PRNG might be usable, it's not going to work well when it comes to people who change out devices, unless you also remember the gaps.  

Make sure you get enough bytes of key (16 is probably about right).  If you can get a device identifier of any sort (a UUID, even just the name of the device), then you can make that the value passed to the HMAC.  NSS has all the pieces you need.
Just noticed I forgot to hg add some files.
Attachment #8566876 - Attachment is obsolete: true
Attachment #8566876 - Flags: feedback?(rjesup)
Attachment #8566876 - Flags: feedback?(bugs)
Attachment #8566956 - Flags: feedback?(rjesup)
Attachment #8566956 - Flags: feedback?(bugs)
Less hot version which people might consider r=me'ing.
Attachment #8566956 - Attachment is obsolete: true
Attachment #8566956 - Flags: feedback?(rjesup)
Attachment #8566956 - Flags: feedback?(bugs)
Attachment #8567215 - Flags: review?(rjesup)
Attachment #8567215 - Flags: review?(bugs)
Comment on attachment 8567215 [details] [diff] [review]
Part 1: enumerateDevices (harmless interface version)

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

::: dom/media/MediaDeviceInfo.cpp
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/MediaDeviceInfo.h"
> +#include "mozilla/dom/MediaStreamBinding.h"
> +#include "mozilla/dom/MediaDeviceInfoBinding.h"

Already included from .h file

@@ +8,5 @@
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/MediaManager.h"
> +#include "nsIEventTarget.h"
> +#include "nsIScriptGlobalObject.h"
> +#include "nsPIDOMWindow.h"

Ditto

::: dom/media/MediaDeviceInfo.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MediaDeviceInfo_h__
> +#define MediaDeviceInfo_h__

"Include guards are named by determining the fully-qualified include path, then substituting _ for / and . and - in it. For example, nsINode.h's guard is nsINode_h, and Element.h's guard is mozilla_dom_Element_h (because its include path is mozilla/dom/Element.h)."

::: dom/media/MediaDevices.cpp
@@ +54,5 @@
> +    uint32_t arrayLen;
> +
> +    nsresult rv;
> +    rv = aDevices->GetAsArray(&elementType, &elementIID, &arrayLen, &rawArray);
> +    NS_ENSURE_SUCCESS(rv, rv);

Prefer when writing new functions (and especially files) to move away from NS_ENSURE_blah.  Especially when raw ptrs are around (though it's safe here)  Not a big deal though

@@ +62,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    // Create array for nsIMediaDevice
> +    nsTArray<nsCOMPtr<nsIMediaDevice> > devices;

I don't think we need > > anymore -- use >>

@@ +70,5 @@
> +      nsCOMPtr<nsIMediaDevice> device(do_QueryInterface(supportsArray[i]));
> +      devices.AppendElement(device);
> +      NS_IF_RELEASE(supportsArray[i]); // explicitly decrease reference count for raw pointer
> +    }
> +    NS_Free(rawArray); // explicitly free for the memory from nsIVariant::GetAsArray

Very tempted since rawArray isn't scoped here to set it to nullptr after this to avoid accidental reuse.  The optimizer will likely remove that in opt builds anyways.

@@ +131,4 @@
>    nsPIDOMWindow* window = GetOwner();
>    nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(window);
>    nsRefPtr<Promise> p = Promise::Create(go, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);

Good catch - is ErrorResult initialized by default?  If not, we may need to deal with this for other revs.  (And if it defaults to success, verify what happens if Create() fails)

::: dom/media/MediaManager.cpp
@@ +1894,5 @@
> +MediaManager::AddWindowID(uint64_t aWindowId)
> +{
> +  // Store the WindowID in a hash table and mark as active. The entry is removed
> +  // when this window is closed or navigated away from.
> +  // This is safe since we're on main-thread, and the windowlist can only

Perhaps add:
 NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
Attachment #8567215 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #10)
> > +#include "mozilla/dom/Promise.h"
> > +#include "mozilla/MediaManager.h"
> > +#include "nsIEventTarget.h"
> > +#include "nsIScriptGlobalObject.h"
> > +#include "nsPIDOMWindow.h"
> 
> Ditto

Thanks, I've removed EventTarget as well, a cut'n'paste snafu.

> Prefer when writing new functions (and especially files) to move away from
> NS_ENSURE_blah.  Especially when raw ptrs are around (though it's safe here)
> Not a big deal though

I'm torn about them. I left it since I thought the other way might entice someone to erroneously add an NS_Free here. The rawPtr is by far the dangerous pattern here. A rewrite of this merging nsIMediaDevice and MediaDeviceInfo would remove the nsIVariant hoop altogether I hope. Gonk would benefit too.

> >    nsRefPtr<Promise> p = Promise::Create(go, aRv);
> > +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> 
> Good catch - is ErrorResult initialized by default?

Yes, to NS_OK.

> If not, we may need to deal with this for other revs.  (And if it defaults to success, verify what
> happens if Create() fails)

It returns nullptr. However, looking at the code [1], the only possible errors I see are NS_ERROR_UNEXPECTED and NS_ERROR_OUT_OF_MEMORY so I'm not sure it can happen (things would need to real bad).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp?rev=a091ec8d2a04&mark=338-362#324
Incorporate feedback. Carrying forward r=jesup.
Attachment #8567215 - Attachment is obsolete: true
Attachment #8567215 - Flags: review?(bugs)
Attachment #8567414 - Flags: review?(bugs)
Change MediaDeviceInfo.label and groupId to not be nullable. See [1]

I'm going to assume jesup is not interested in this webidl-only change, and carry forward his r+.

[1] https://github.com/w3c/mediacapture-main/issues/141
Attachment #8567414 - Attachment is obsolete: true
Attachment #8567414 - Flags: review?(bugs)
Attachment #8567441 - Flags: review?(bugs)
Attachment #8567418 - Attachment description: Part 2: enumerateDevices (super-cookie version) for testing only (2) → Part 3: enumerateDevices (super-cookie version) for testing only (2)
Comment on attachment 8567445 [details] [diff] [review]
Part 2: enumerateDevices returns label for active gUM windows

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

::: dom/media/MediaDevices.cpp
@@ +42,5 @@
>  public:
>    NS_DECL_ISUPPORTS
>  
> +  explicit EnumDevResolver(Promise* aPromise, uint64_t aWindowId)
> +  : mPromise(aPromise), mWindowId(aWindowId) {}

drop explicit now

@@ +90,5 @@
> +        // Include name only if page currently has a gUM stream active.
> +        if (MediaManager::Get()->IsWindowActivelyCapturing(mWindowId)) {
> +          device->GetName(name);
> +        }
> +        // TODO: return anonymized origin-persistent id (1046245)

I can't let you land code without that.  I think that I'd be ok if you changed the ID to "placeholder-id" or something like that if you really have to land this code.  Either that, or a part 3 that actually had content :)
Attachment #8567445 - Flags: review?(martin.thomson)
As I commented on github, I think that you should probably check with permission manager as an alternative to checking for active streams.  Less conservative perhaps, but it's not like they couldn't easily engineer a situation where they had an active stream...
Attachment #8567446 - Attachment is obsolete: true
Attachment #8567568 - Flags: feedback?(martin.thomson)
Comment on attachment 8567568 [details] [diff] [review]
Part 3: enumerateDevices hmac id WIP

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

I'd be reasonably happy to see this land in a form like this (with persistence, of course), but I think that this could be changed slightly and it will make your later work easier.

::: dom/media/MediaDevices.cpp
@@ +168,5 @@
> +
> +        // deviceId would be a supercookie if we returned it. Anonymize it:
> +        // 1. Make a uuid for this origin (TODO: store it in a global list),
> +        // 2. Return hmac_sha1(uuid, id) - an anonymized id unique to origin.
> +        // TODO: Move off main thread!

Running a few hashes on the main thread shouldn't be an issue.  It will probably take longer to do the dispatches back and forth.  Most CPUs can do 10s or 100s of thousands of hashes per second. 

That said, I think that it will make the implementation of the constraint easier if this work were done on the MediaManager thread in GetUserMediaDevicesTask::Run: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1371

This code on the main thread probably needs to extract the preference state for the origin as well as the per-origin key and then the media manager thread can do all the heavy lifting.

@@ +188,5 @@
> +        PR_snprintf(hex, sizeof(hex),
> +            "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x"
> +            "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x",
> +            m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8], m[9], m[10],
> +            m[11], m[12], m[13], m[14], m[15], m[16], m[17], m[18], m[19]);

Truncating this to 16 bytes is safe, you might even go as low as 12 with a very low risk of collision.

@@ +189,5 @@
> +            "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x"
> +            "%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x",
> +            m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8], m[9], m[10],
> +            m[11], m[12], m[13], m[14], m[15], m[16], m[17], m[18], m[19]);
> +        NS_ConvertUTF8toUTF16 anonId(hex);

It might pay to split the whole ID anonymization section out given how big it is now.
Attachment #8567568 - Flags: feedback?(martin.thomson) → feedback+
(In reply to Martin Thomson [:mt] from comment #20)
> I think that it will make the implementation of the constraint
> easier if this work were done on the MediaManager thread

What constraint? I'm not following. Moving it there makes sense though for implementing the global list, as I understand write to file should not be done on the main thread, and I assume I can write to file on the MediaManager thread.
The constraint I refer to is the deviceId constraint.  Moving the anonymization to the MediaManager thread will ensure that it is in a place that is common to both enumerate and gUM.
Ah, good point.
Moved off main thread.
Attachment #8567568 - Attachment is obsolete: true
Comment on attachment 8567441 [details] [diff] [review]
Part 1: enumerateDevices (harmless interface version) (3) r=jesup

>+[Func="Navigator::HasUserMediaSupport"]
>+interface MediaDeviceInfo {
>+  readonly    attribute DOMString       deviceId;
>+  readonly    attribute MediaDeviceKind kind;
>+  readonly    attribute DOMString      label;
>+  readonly    attribute DOMString      groupId;
Want to align the attribute names properly and remove extra spaces between
readonly and attribute


>+};
>\ No newline at end of file
and possibly add a new line.


> [Func="Navigator::HasUserMediaSupport"]
> interface MediaDevices : EventTarget {
>-//    attribute EventHandler ondevicechange;
>-//
>-//    void enumerateDevices (MediaDeviceInfoCallback resultCallback);
>-//
>+//  attribute EventHandler ondevicechange;
> //  static Dictionary getSupportedConstraints (DOMString kind);
> 
>-  [Throws, Func="Navigator::HasUserMediaSupport"]
>+  [Throws]
>+  Promise<sequence<MediaDeviceInfo>> enumerateDevices ();
extra space between s and (). 
(looks like it is in the spec too)
Attachment #8567441 - Flags: review?(bugs) → review+
Incorporate feedback. Carrying forward r=smaug, jesup.
Attachment #8567441 - Attachment is obsolete: true
Attachment #8567714 - Flags: review+
Incorporate feedback, and check persistent permissions.
Attachment #8567445 - Attachment is obsolete: true
Adds in-memory lookup of uuids by origin. Loading and saving to file in Part 4.
Attachment #8567649 - Attachment is obsolete: true
Attachment #8567772 - Flags: feedback?
Attachment #8567769 - Flags: feedback?(martin.thomson)
Attachment #8567772 - Flags: feedback? → feedback?(martin.thomson)
Comment on attachment 8567772 [details] [diff] [review]
Part 3: enumerateDevices hmac id persisted within-session (3)

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

A few little concerns, but generally good.

::: dom/media/MediaManager.cpp
@@ +1379,5 @@
> +  // Cribbed from nricectx.cpp
> +  static nsresult
> +  hmac_sha1(const char *key, int keyl, const char *buf, int bufl,
> +            unsigned char *result) {
> +    CK_MECHANISM_TYPE mech = CKM_SHA_1_HMAC;

I'd inline this one (or at least mark it const).  I also prefer SHA-256, but here it matters little.  HMAC-SHA1 has pretty good pre-image resistance still.

@@ +1455,5 @@
> +          NS_ENSURE_SUCCESS(rv, rv);
> +          rv = uuidgen->GenerateUUIDInPlace(&id);
> +          NS_ENSURE_SUCCESS(rv, rv);
> +        }
> +        id.ToProvidedString(uuid);

This has been itching at me now a while.  The problem with the UUID is that this code takes the UUID and stores it in a string that is longer than a single block of the hash function, which means that the HMAC is quite a lot less efficient.  Is there any reason you can't just crank out some random bytes directly?

This actually looks easier: https://dxr.mozilla.org/mozilla-central/source/dom/base/Crypto.cpp#137

As long as you don't need to display the result (I don't see any need to), or store it somewhere that can't save some binary data, this is going to be easier.

@@ +1459,5 @@
> +        id.ToProvidedString(uuid);
> +      }
> +      originUuid = new OriginUuid(uuid, false);
> +      mManager->mOriginUuids.Put(origin, originUuid);
> +    }

needs moar functions.

@@ +1537,5 @@
> +        rv = AnonymizeId(id, mOrigin);
> +        if (NS_FAILED(rv)) {
> +          break;
> +        }
> +        source->SetId(id);

This looks like a case for:

if (!mPrivileged) {
  for (auto& source : *result) {
    rv = source->AnonymizeId();
    if (NS_FAILED(...
  }
}

Or you can move the get and set into the Anonymize function.

@@ +1987,5 @@
>      Preferences::GetCString("media.video_loopback_dev");
>  
> +  nsCString origin;
> +  nsPrincipal::GetOriginForURI(aWindow->GetDocumentURI(),
> +                               getter_Copies(origin));

Double-check this, but I think that you need to pull the principal from the window or the current JS context, rather than look at the URI.
Attachment #8567772 - Flags: feedback?(martin.thomson) → feedback+
Unbitrot.
Attachment #8567714 - Attachment is obsolete: true
Attachment #8567776 - Flags: review+
Attachment #8567769 - Flags: feedback?(martin.thomson) → feedback+
Attachment #8567769 - Flags: review?(martin.thomson)
(In reply to Martin Thomson [:mt] from comment #29)
> > +        id.ToProvidedString(uuid);
> 
> This has been itching at me now a while.  The problem with the UUID is that
> this code takes the UUID and stores it in a string that is longer than a
> single block of the hash function, which means that the HMAC is quite a lot
> less efficient.

Efficient as in time it takes to compute? If so, then I doubt it will matter in practice. Can we optimize later? How long should it be?

> Is there any reason you can't just crank out some random
> bytes directly?
> 
> This actually looks easier:
> https://dxr.mozilla.org/mozilla-central/source/dom/base/Crypto.cpp#137
> 
> As long as you don't need to display the result (I don't see any need to),
> or store it somewhere that can't save some binary data, this is going to be
> easier.

Could do that, but that would have to wait till tomorrow. No chance of landing this I suppose? Maybe too late. But if not, feel free to checkin-needed should you decide to r+ all of these and try is green. ;-) Not sure what time it is where you are.

> This looks like a case for:
> 
> if (!mPrivileged) {
>   for (auto& source : *result) {
>     rv = source->AnonymizeId();
>     if (NS_FAILED(...
>   }
> }
> 
> Or you can move the get and set into the Anonymize function.

That's too tightly coupled for my taste. In any case, I smell a larger cleanup here anyway once nsIMediaDevice and MediaDeviceInfo can be merged.

> > +  nsPrincipal::GetOriginForURI(aWindow->GetDocumentURI(),
> > +                               getter_Copies(origin));
> 
> Double-check this, but I think that you need to pull the principal from the
> window or the current JS context, rather than look at the URI.

If this is true, then our existing loop and screensharing checks are in poor shape:
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=ae5b86a2529d#1661
Added const. Additional fixes will have to wait (I'd need to understand the significance of having the right hmac input length, and), I'm turning in.
Attachment #8567772 - Attachment is obsolete: true
Attachment #8567840 - Flags: review?(martin.thomson)
Comment on attachment 8567828 [details] [diff] [review]
Part 4: enumerateDevices origin-unique ids persisted to disk

Choice of approach for persisting was informed by [1] and [2]. Thanks!

[1] https://wiki.mozilla.org/Performance/Avoid_SQLite_In_Your_Next_Firefox_Feature#How_to_Store_Your_Data
[2] https://hg.mozilla.org/mozilla-central/rev/b682e16c46f2
Unbitrot. Noticed I bitrotted part 4 with 3. I also need to fix the Windows template problem on try.
Attachment #8567828 - Attachment is obsolete: true
Attachment #8567828 - Flags: review?(martin.thomson)
Attachment #8567893 - Flags: review?(martin.thomson)
Whooped by an NS_IMETHODIMP macro (builds on Windows again).

Win try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec302aa3cc3
Attachment #8567840 - Attachment is obsolete: true
Attachment #8567840 - Flags: review?(martin.thomson)
Attachment #8567904 - Flags: review?(martin.thomson)
Attachment #8567769 - Flags: review?(rjesup)
Attachment #8567904 - Flags: review?(rjesup)
Comment on attachment 8567893 [details] [diff] [review]
Part 4: enumerateDevices origin-unique ids persisted to disk (2)

Reaching out to jesup due to timezone difference, since Martin had indicated in his feedback+'es his main objection to landing was absence of part 4, which is now here.
Attachment #8567893 - Flags: review?(rjesup)
Missed a small part: Don't save origin-ids in private browsing to disk.

New try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=10d92d1b6cbd
Attachment #8567893 - Attachment is obsolete: true
Attachment #8567893 - Flags: review?(rjesup)
Attachment #8567893 - Flags: review?(martin.thomson)
Attachment #8567950 - Flags: review?(rjesup)
Attachment #8567950 - Flags: review?(martin.thomson)
Attachment #8567904 - Flags: review?(rjesup) → review+
Attachment #8567769 - Flags: review?(rjesup) → review+
Comment on attachment 8567893 [details] [diff] [review]
Part 4: enumerateDevices origin-unique ids persisted to disk (2)

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

::: dom/media/MediaManager.cpp
@@ +1426,5 @@
> +      status = PK11_DigestOp(hmac_ctx, unconst_uchar_cast(buf), bufl);
> +      if (status != SECSuccess) {
> +        goto abort;
> +      }
> +      status = PK11_DigestFinal(hmac_ctx, result, &hmac_len, 20);

Hoist the '20's in this file to a #define

@@ +1456,4 @@
>      {
> +      // deviceId would be a supercookie if we returned it. Anonymize it:
> +      // 1. Get (or create) a persistent uuid for this origin.
> +      // 2. Return hmac_sha1(uuid, id) - an anonymized id unique to origin.

Consider referencing the spec here

@@ +1474,5 @@
> +                do_GetService("@mozilla.org/uuid-generator;1", &rv);
> +            NS_ENSURE_SUCCESS(rv, rv);
> +            rv = uuidgen->GenerateUUIDInPlace(&id);
> +            NS_ENSURE_SUCCESS(rv, rv);
> +          }

unnecessary scoping

@@ +1476,5 @@
> +            rv = uuidgen->GenerateUUIDInPlace(&id);
> +            NS_ENSURE_SUCCESS(rv, rv);
> +          }
> +          id.ToProvidedString(uuid);
> +        }

ditto

@@ +1487,5 @@
> +      {
> +        NS_ConvertUTF16toUTF8 id(aId);
> +        hmac_sha1(originUuid->mUuid.get(), originUuid->mUuid.Length(),
> +                  id.get(), id.Length(), mac);
> +      }

ditto

@@ +1562,5 @@
> +        if (f >= 0) {
> +          const nsACString& uuid = Substring(line, 0, f);
> +          const nsACString& origin = Substring(line, f+1);
> +          mManager->mOriginUuids.Put(origin, new OriginUuid(uuid, false));
> +        }

And so what happens if there is no space?  At least comment here

@@ +1625,5 @@
> +      }
> +      return NS_OK;
> +    }
> +
> +    nsresult Load()

At least comment as to which thread these occur on

@@ +1660,5 @@
> +      }
> +      return NS_OK;
> +    }
> +
> +    bool mUnsaved;

I presume this is intentionally public?
Attachment #8567893 - Attachment is obsolete: false
Comment on attachment 8567950 [details] [diff] [review]
Part 4: enumerateDevices origin-unique ids persisted to disk (3)

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

see comments on previous patch
Attachment #8567950 - Flags: review?(rjesup) → review+
Attachment #8567893 - Attachment is obsolete: true
(In reply to Randell Jesup [:jesup] from comment #40)
> > +    bool mUnsaved;
> 
> I presume this is intentionally public?

Yes it is accessed in Run() as: if (loader.mUnsaved) { loader.Save(); }
Update commit msg. Carrying forward r=jesup.
Attachment #8567769 - Attachment is obsolete: true
Attachment #8567769 - Flags: review?(martin.thomson)
Attachment #8567987 - Flags: review+
Update commit msg. Carrying forward r=jesup.
Attachment #8567904 - Attachment is obsolete: true
Attachment #8567904 - Flags: review?(martin.thomson)
Attachment #8567988 - Flags: review+
Incorporate feedback. Add back a const merge-mistake of mine of Part 3 from Martin's feedback. Carrying forward r=jesup
Attachment #8567950 - Attachment is obsolete: true
Attachment #8567950 - Flags: review?(martin.thomson)
Attachment #8567990 - Flags: review+
Thanks!
Keywords: checkin-needed
Removing checkin-needed until we have a way to clear deviceId cache.
Keywords: checkin-needed
Comment on attachment 8568537 [details] [diff] [review]
Part 5: sanitize origin-specific deviceIds when user clears cookies

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

Please request review from a browser/content peer (perhaps Florian)

::: browser/base/content/sanitize.js
@@ +201,5 @@
>  
> +        // Clear deviceIds.
> +        var mediaMgr = Components.classes["@mozilla.org/mediaManagerService;1"]
> +                                 .getService(Ci.nsIMediaManagerService);
> +        mediaMgr.sanitizeDeviceIds(this.range && this.range[0]);

Need a different peer for this (florian?)

::: dom/media/MediaManager.cpp
@@ +1376,5 @@
>  #define HMAC_LENGTH 20
>  
>  /**
> + * Common base for chrome-only functions GetUserMediaDevices & SanitizeDeviceIds.
> + * Comtains Enumerates a list of audio & video devices,

Get rid of "Comtains" and re-wrap the comment

@@ +1382,5 @@
>   * callback.
>   *
>   * All code in this class runs on the MediaManager thread.
>   */
> +class MediaManagerTask : public Task

MediaManagerDevicesTask perhaps?

@@ +1682,5 @@
> +
> +    void Clear(int64_t aSinceWhen)
> +    {
> +      // Avoid int64_t* <-> void* casting offset
> +      OriginUuid since(nsCString(), aSinceWhen  / 1000000, false);

There should be a define for 1000000 (PR_USECS_PER_SEC or some such).  Not critical, but makes the units explicit and avoids possible typos
Attachment #8568537 - Flags: review?(rjesup) → review+
Attachment #8568537 - Flags: review?(florian)
Severity: normal → major
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [gUM]
Comment on attachment 8568537 [details] [diff] [review]
Part 5: sanitize origin-specific deviceIds when user clears cookies

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

r- because per the discussion we just had in #media, it seems this isn't e10s ready.

The changes in sanitize.js looked good though, if we assume it's OK to sanitize on the parent process a file that's written by the child process.

::: browser/base/content/sanitize.js
@@ +199,5 @@
>            cookieMgr.removeAll();
>          }
>  
> +        // Clear deviceIds.
> +        var mediaMgr = Components.classes["@mozilla.org/mediaManagerService;1"]

nit: Use "let" instead of "var" in new code (I know this method is already using let/var inconsistently though; so it doesn't really make a difference here).
Attachment #8568537 - Flags: review?(florian) → review-
Rank: 10
Blocks: 1138851
Blocks: 1027582
Getting it off my laptop. Works, just need to wire in SanitizeDeviceId again.
Unbitrot.
Attachment #8567776 - Attachment is obsolete: true
Attachment #8575036 - Flags: review+
Unbitrot.
Attachment #8567988 - Attachment is obsolete: true
Attachment #8575039 - Flags: review+
Unbitrot.
Attachment #8567990 - Attachment is obsolete: true
Attachment #8575040 - Flags: review+
Works with e10s and off main thread.
Attachment #8568537 - Attachment is obsolete: true
Attachment #8574278 - Attachment is obsolete: true
Attachment #8575049 - Flags: review?(dkeeler)
Sets up GetOriginKeys as a main process PMedia service. Service persists per-origin keys to disk for internal use (hashing persistent deviceIds exposed to content).
Attachment #8575056 - Flags: review?(wmccloskey)
Attachment #8575056 - Attachment description: e10s proxy for enumerateDevices (2) → Part 6: e10s proxy for enumerateDevices (2)
Comment on attachment 8575056 [details] [diff] [review]
Part 6: e10s proxy for enumerateDevices (2)

Almost forgot, this patch also handles private browsing better, using a separate set of keys that are never saved to disk.
Attachment #8575056 - Flags: review?(rjesup)
Incorporate feedback. e10s ready. File is written on single thread in main process.
Attachment #8575063 - Flags: review?(florian)
Attachment #8575063 - Flags: review?(rjesup)
Comment on attachment 8575056 [details] [diff] [review]
Part 6: e10s proxy for enumerateDevices (2)

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

::: dom/media/systemservices/MediaChild.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/Assertions.h"

see MediaParent.cpp comments

::: dom/media/systemservices/MediaParent.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/Assertions.h"

Is this actually needed before MediaParent.h?  If so, perhaps it belongs in there

@@ +59,5 @@
> +      // Make a new one
> +      nsresult rv;
> +      nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +          do_GetService("@mozilla.org/uuid-generator;1", &rv);
> +      NS_ENSURE_SUCCESS(rv, rv);

New code - avoid NS_ENSURE_*

@@ +113,5 @@
> +
> +  nsresult
> +  GetOriginUuid(const nsACString& aOrigin, nsCString& result)
> +  {
> +    uint32_t before = mIds.Count();

auto please, as I just filed a bug on nsCOMArray's returning uint32_t for Count/Length instead of what nsTArray uses
Attachment #8575056 - Flags: review?(rjesup) → review+
Comment on attachment 8575063 [details] [diff] [review]
Part 7: sanitize origin-specific deviceIds when user clears cookies (2)

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

::: dom/media/systemservices/MediaParent.cpp
@@ +52,5 @@
> +  class OriginUuidsTable
> +  {
> +  public:
> +    OriginUuidsTable() {}
> +    

trailing spaces in this patch

@@ +62,5 @@
> +        // Make a new one
> +        nsresult rv;
> +        nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +        do_GetService("@mozilla.org/uuid-generator;1", &rv);
> +        NS_ENSURE_SUCCESS(rv, rv);

See comments on patch 6

@@ +116,5 @@
> +    
> +    nsresult
> +    GetOriginUuid(const nsACString& aOrigin, nsCString& result)
> +    {
> +      uint32_t before = mIds.Count();

See comments on patch 6
Attachment #8575063 - Flags: review?(rjesup) → review+
Attachment #8575063 - Flags: review?(florian) → review+
Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7df185eff48

Looks like I need to fix cleaning of history on shutdown. MediaManager is likely gone at that point, I think is the problem. I now have two reasons to put sanitizeDeviceIds somewhere other than nsIMediaManagerService. The other being that opening MediaManager on the main process just to pass some words through it, is probably not great long-term.
Comment on attachment 8575049 [details] [diff] [review]
Part 5: enumerateDevices using nsICryptoHMAC interface

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

I reviewed the crypto bits. I imagine a dom/media peer should have a look for style/code conventions/etc. that I may not be aware of in this area of the code.
I'm concerned about the key material used in the HMAC (see below). r=me with that addressed.

::: dom/media/MediaManager.cpp
@@ +331,5 @@
>      mOnFailure.swap(aOnFailure);
>    }
>  
> +  nsresult
> +  AnonymizeId(nsAString& aId, const nsACString& aOriginUuid)

It looks like aOriginUuid is a string of the form "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}". It would be best to convert this to the actual bytes of the key we want to use, rather than a hex representation (or, since this came from a nsID, use those bytes directly, although see the later comment regarding how this key material gets generated).

@@ +337,5 @@
> +    nsCString mac;
> +    NS_ConvertUTF16toUTF8 id(aId);
> +    nsresult rv;
> +    nsCOMPtr<nsIKeyObjectFactory> factory =
> +    do_CreateInstance("@mozilla.org/security/keyobjectfactory;1", &rv);

nit: usually I see lines like this indented two spaces
Also, looks like this should be do_GetService instead of do_CreateInstance.

@@ +349,5 @@
> +      return rv;
> +    }
> +
> +    nsCOMPtr<nsICryptoHMAC> hasher =
> +    do_CreateInstance(NS_CRYPTO_HMAC_CONTRACTID, &rv);

nit: same here regarding indentation

@@ +353,5 @@
> +    do_CreateInstance(NS_CRYPTO_HMAC_CONTRACTID, &rv);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    rv = hasher->Init(nsICryptoHMAC::SHA1, key);

SHA256 would probably be better, unless a specification conflicts with that

@@ +1453,5 @@
>              do_GetService("@mozilla.org/uuid-generator;1", &rv);
>          NS_ENSURE_SUCCESS(rv, rv);
>  
>          nsID nsid;
>          rv = uuidgen->GenerateUUIDInPlace(&nsid);

Looking at the implementation for this, it doesn't seem like we have a good enough guarantee of randomness that we should use this as a key directly. We should either do some sort of key-derivation before using it in AnonymizeId or we should use something that in theory is sufficiently random (e.g. nsIRandomGenerator::GenerateRandomBytes).
Attachment #8575049 - Flags: review?(dkeeler) → review+
Comment on attachment 8575056 [details] [diff] [review]
Part 6: e10s proxy for enumerateDevices (2)

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

It looks like GetOriginKey doesn't need to be synchronous. The only use that I see is in MediaManager, and we just dispatch a runnable with the result right after that. So I'd like to see a version of the patch where we don't spin a nested event loop.

I also wonder if PMedia is really needed. It would be a lot easier to stick GetOriginKey directly on PBackground. Are you planning to add more stuff to PMedia later? If so, then this structure makes sense. If not, then I don't think we need PMedia.

::: dom/media/systemservices/MediaChild.cpp
@@ +14,5 @@
> +
> +PRLogModuleInfo *gMediaChildLog;
> +
> +#undef LOG
> +#undef LOG_ENABLED

All of this logging goop seems unnecessary. LOG_ENABLED is never used in any of these files even though it's duplicated in each one. LOG is used, but you could almost as easily use PR_LOG (which is already #defined to nothing when PR_LOGGING is undefined).

@@ +45,5 @@
> +    }
> +    // By now PBackground is guaranteed to be up
> +    MOZ_RELEASE_ASSERT(existingBackgroundChild);
> +    // Create PMedia by sending a message to the parent
> +    sMedia = existingBackgroundChild->SendPMediaConstructor();

I'm worried about thread safety. Will GetOriginKey always be called from the same thread in a given process? If so, we should assert that. If not, then that's a problem.

@@ +52,5 @@
> +  MOZ_ASSERT(sMedia);
> +  return static_cast<MediaChild*>(sMedia);
> +}
> +
> +bool GetOriginKey(const nsCString& aOrigin, bool aPrivateBrowsing, nsCString& aKey)

bool should be on a separate line.

@@ +54,5 @@
> +}
> +
> +bool GetOriginKey(const nsCString& aOrigin, bool aPrivateBrowsing, nsCString& aKey)
> +{
> +  Media()->SendGetOriginKey(aOrigin, aPrivateBrowsing, &aKey);

As far as I can tell, we're never on the main thread when this runs. Can you assert that? It's important because the message is sync.

@@ +69,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +void Shutdown()

void on separate line. However, I don't think this Shutdown stuff is needed. It looks like CloseForCurrentThread is automatically called on all NSPR threads. Unless you have a reason for it, please remove.

@@ +91,5 @@
> +
> +  MOZ_COUNT_CTOR(MediaChild);
> +}
> +
> +MediaChild::~MediaChild()

Please null out sMedia in here.

@@ +100,5 @@
> +
> +  MOZ_COUNT_DTOR(MediaChild);
> +}
> +
> +PMediaChild* CreateMediaChild() {

return type on separate line.

::: dom/media/systemservices/MediaParent.cpp
@@ +338,5 @@
> +    gMediaParentLog = PR_NewLogModule("MediaParent");
> +#endif
> +  LOG(("MediaParent: %p", this));
> +
> +  mPBackgroundThread = NS_GetCurrentThread();

What's this for?

@@ +339,5 @@
> +#endif
> +  LOG(("MediaParent: %p", this));
> +
> +  mPBackgroundThread = NS_GetCurrentThread();
> +  sMediaParent = this;

And what's this for?

@@ +351,5 @@
> +
> +  MOZ_COUNT_DTOR(MediaParent);
> +}
> +
> +PMediaParent* CreateMediaParent() {

Brace and return value on their own lines.

::: dom/media/systemservices/MediaParent.h
@@ +34,5 @@
> +  class OriginUuidsTable;
> +  class OriginUuidsLoader;
> +
> +  ScopedDeletePtr<OriginUuidsLoader> mOriginUuids;
> +  ScopedDeletePtr<OriginUuidsTable> mPrivateBrowsingOriginUuids;

This will create a separate set of UUIDs per MediaParent, which I think is not what you want. We'll have one MediaParent per process, including the chrome process. We might load the same origin in two processes, and I'm guessing they should share a UUID in that case. Maybe these should just be static?

::: dom/media/systemservices/MediaUtils.cpp
@@ +12,5 @@
> +#include "mozilla/ipc/PBackgroundChild.h"
> +#include "nsIIPCBackgroundChildCreateCallback.h"
> +
> +// NSPR_LOG_MODULES=MediaBroker:5
> +PRLogModuleInfo *gMediaBrokerLog = nullptr;

This logging doesn't seem to be used.

::: dom/media/systemservices/moz.build
@@ +19,5 @@
> +                        'MediaParent.cpp',
> +                        'MediaUtils.cpp'
> +    ]
> +    IPDL_SOURCES = [
> +        'PMedia.ipdl',

Given that PBackground refers to PMedia without any kind of #ifdef MOZ_WEBRTC, I think this will effectively make compiling without MOZ_WEBRTC impossible. I don't know how important that is.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +290,5 @@
> +
> +bool
> +BackgroundChildImpl::DeallocPMediaChild(media::PMediaChild *aActor)
> +{
> +  MOZ_ASSERT(aActor);

The assertion here really isn't necessary.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +369,5 @@
> +
> +bool
> +BackgroundParentImpl::DeallocPMediaParent(media::PMediaParent *aActor)
> +{
> +  MOZ_ASSERT(aActor);

Same here.
Attachment #8575056 - Flags: review?(wmccloskey) → review-
Comment on attachment 8575056 [details] [diff] [review]
Part 6: e10s proxy for enumerateDevices (2)

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

::: dom/media/systemservices/MediaUtils.cpp
@@ +26,5 @@
> +
> +namespace mozilla {
> +namespace media {
> +
> +class WorkerBackgroundChildCallback MOZ_FINAL :

Probably not "Worker" here... cloned from RuntimeService.cpp?  Perhaps MediaBackgroundChildCallback?  Or factor this out of RuntimeService and here and let everyone use one impl...
Unbitrot.
Attachment #8575036 - Attachment is obsolete: true
Attachment #8579467 - Flags: review+
Unbitrot.
Attachment #8575039 - Attachment is obsolete: true
Attachment #8579473 - Flags: review+
Unbitrot.
Attachment #8575040 - Attachment is obsolete: true
Attachment #8579474 - Flags: review+
Incorporate feedback. Carrying forward r=keeler.
- Updated key material used in HMAC to 18 bytes of random data.
- Updated from SHA1 to SHA256.
- Fix indentation.
Attachment #8575049 - Attachment is obsolete: true
Attachment #8579483 - Flags: review+
Unbitrot for now.
Attachment #8575056 - Attachment is obsolete: true
Attachment #8579491 - Flags: review-
Comment on attachment 8579483 [details] [diff] [review]
Part 5: enumerateDevices using nsICryptoHMAC interface (2) r=keeler

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

Looks like this needs a few more tweaks.

::: dom/media/MediaManager.cpp
@@ +357,5 @@
> +      do_CreateInstance(NS_CRYPTO_HMAC_CONTRACTID, &rv);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    rv = hasher->Init(nsICryptoHMAC::SHA1, key);

This says SHA1 still.

@@ +372,5 @@
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +
> +    aId = NS_ConvertUTF8toUTF16(mac);

The output of the hash is going to be random bytes, which aren't always valid UTF-8.  I think that you are missing a Base64Encode somewhere here.

@@ +1482,5 @@
>      // Format of file is (first line is version #):
>      //
>      // 1
>      // {54b1d3ad-18ad-0546-a551-80c1bf425057} http://fiddle.jshell.net
>      // {f9c2a045-53b4-0546-bdd0-b96f41850f8a} http://mozilla.github.io

Fix this if you are using base64
Straight-up merge of parts 5, 6 and 7. Reviews carried forward are for area-specific sub-parts only (crypto and sanitize.js respectively).

Too much overlap between patches made working on them untenable. This should make upcoming reviews more pleasant.
Attachment #8579483 - Attachment is obsolete: true
Attachment #8579491 - Attachment is obsolete: true
Attachment #8579493 - Attachment is obsolete: true
Attachment #8579500 - Flags: review-
Incorporate some feedback.
- Added thread assertions.
- Removed some logging goop.
- return type on separate line, braces etc.
- removed unused stuff, shutdown etc.
- Singleton was already added by patch 6.
- compiles with --disable-webrtc.
- fix SHA1 to SHA256. Thanks Martin, a patch merge snafu!

IPC methods are still sync and I'll follow up with a question on that.

Still looking for a "clear history on shutdown solution".
Attachment #8579500 - Attachment is obsolete: true
(In reply to Bill McCloskey (:billm) from comment #64)
> It looks like GetOriginKey doesn't need to be synchronous. The only use that
> I see is in MediaManager, and we just dispatch a runnable with the result
> right after that. So I'd like to see a version of the patch where we don't
> spin a nested event loop.

Wouldn't I need to stash my mOnSuccess and mOnFailure JS callbacks somewhere, since AFAIK they can't be serialized over to the parent process?

Would it be OK to leave it sync in this case? MediaChild is always on MediaManager.mMediaThread, which I'm asserting now. Apart from some timers that run our fake (solid color) video and fake (sine) audio, there's no real-time stuff on that thread, only on-demand enumeration for getUserMedia requests.

> I also wonder if PMedia is really needed. It would be a lot easier to stick
> GetOriginKey directly on PBackground. Are you planning to add more stuff to
> PMedia later? If so, then this structure makes sense. If not, then I don't
> think we need PMedia.

I see us needing to add more e10s stuff to media going forward, so I've left it for now. I don't have specifics in mind though.

> I'm worried about thread safety. Will GetOriginKey always be called from the
> same thread in a given process? If so, we should assert that.

Yes and I've added assertions now.

> This will create a separate set of UUIDs per MediaParent, which I think is
> not what you want. We'll have one MediaParent per process, including the
> chrome process. We might load the same origin in two processes, and I'm
> guessing they should share a UUID in that case. Maybe these should just be
> static?

I added a singleton in Part 6, which I should have included you on. Feel free to take a look. Now that parts 5, 6 and 7 are merged it should be easier to review the thing (comment 75).

> Given that PBackground refers to PMedia without any kind of #ifdef
> MOZ_WEBRTC, I think this will effectively make compiling without MOZ_WEBRTC
> impossible. I don't know how important that is.

Thanks, fixed now.


A final wrinkle:
----------------

Part 6 adds another sync method, SanitizeOriginKeys, which is called by sanitize.js on the main process when users clear history in about:preferences. Right now I'm dispatching this to the MediaManager.mMediaThread as well to avoid locking (and abide by the thread asserts), but this opens the thread on the main process which is not great long term, as it sticks around right now and we want to get better at cleaning it up (existing bug). Looking for other ideas there.

I've learned that for users who check the "clear history on shutdown" box in about:preferences, that sanitize.js also invokes MediaChild.SanitizeOriginKeys on late shutdown and early startup, which seems to be causing pain that I'm still investigating.
Flags: needinfo?(wmccloskey)
(In reply to Martin Thomson [:mt] from comment #73)
> This says SHA1 still.

Yes saw your message on that as I was attaching the next patch, so it should be in there now.

> > +    aId = NS_ConvertUTF8toUTF16(mac);
> 
> The output of the hash is going to be random bytes, which aren't always
> valid UTF-8.  I think that you are missing a Base64Encode somewhere here.

Good catch! A gotcha with our habit of storing blobs in nsCStrings.

I'll update the patch to Base64Encode it.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #77)
> I'll update the patch to Base64Encode it.

Nevermind, it turns out the 'true' argument to rv = hasher->Finish(true, mac); means it returns ascii, so we're good. That explains why I'm seeing good ids.
> Wouldn't I need to stash my mOnSuccess and mOnFailure JS callbacks somewhere, since AFAIK
> they can't be serialized over to the parent process?

The callbacks are already stored on the DeviceSuccessCallbackRunnable. You'd be doing almost the exact same thing. You just need a new runnable, call it OriginKeyRunnable, similar to DeviceSuccessCallbackRunnable, that would be passed to GetOriginKey. GetOriginKey would invoke the GetOriginKeyRunnable runnable after the PBackground for the thread is ready and SendGetOriginKey returns. Then the GetOriginKeyRunnable would dispatch the DeviceSuccessCallbackRunnable on the main thread.

It's a bit more complicated, but it avoids the nested event loop. Even though it's not a performance problem here, nested event loops can create all sorts of problems like deadlocks and stack overflows that we should avoid, especially in cases like this where it's easy to do so.

> Part 6 adds another sync method, SanitizeOriginKeys, which is called by sanitize.js on the
> main process when users clear history in about:preferences.

I think the sanitizeDeviceIds function should be asynchronous, even if we're going to use it synchronously from sanitize.js. It would be passed a callback that would be invoked when the sanitization is complete.
Flags: needinfo?(wmccloskey)
Comment on attachment 8579510 [details] [diff] [review]
Parts 5-7: enumerateDevices on e10s w/nsICryptoHMAC + clear cookies (2) r=keeler, florian

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

::: dom/media/MediaManager.cpp
@@ +1428,5 @@
>  
> +  void // NS_IMETHOD
> +  Run()
> +  {
> +    NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");

Use MOZ_ASSERT for this please.

::: dom/media/systemservices/MediaChild.cpp
@@ +22,5 @@
> +namespace mozilla {
> +namespace media {
> +
> +static PMediaChild* sMedia;
> +nsCOMPtr<nsIThread> sMediaChildThread;

sMediaChildThread doesn't seem to be used.

@@ +66,5 @@
> +  LOG(("SanitizeOriginKeys since %llu", aSinceWhen));
> +  return true;
> +}
> +
> +class ShutdownRunnable : public nsRunnable {

I think you meant to remove this. It doesn't appear to be used.

@@ +87,5 @@
> +  LOG(("MediaChild: %p", this));
> +  MOZ_COUNT_CTOR(MediaChild);
> +}
> +
> +MediaChild::~MediaChild()

I still think you should null out sMedia here.

::: dom/media/systemservices/MediaChild.h
@@ +15,5 @@
> +namespace media {
> +
> +bool GetOriginKey(const nsCString& aOrigin, bool aPrivateBrowsing, nsCString& aKey);
> +bool SanitizeOriginKeys(const uint64_t& aSinceWhen);
> +void Shutdown();

This function is never defined.

::: dom/media/systemservices/MediaParent.cpp
@@ +374,5 @@
> +  LOG((__PRETTY_FUNCTION__));
> +}
> +
> +MediaParent::MediaParent()
> +  : mSingleton (MediaParentSingleton::Get())

No space after mSingleton.

@@ +392,5 @@
> +
> +  MOZ_COUNT_DTOR(MediaParent);
> +}
> +
> +PMediaParent* CreateMediaParent() {

Brace on its own line.

::: dom/media/systemservices/moz.build
@@ +51,5 @@
> +                    'MediaParent.cpp',
> +                    'MediaUtils.cpp'
> +]
> +IPDL_SOURCES += [
> +    'PMedia.ipdl',

Isn't this included twice?

@@ +53,5 @@
> +]
> +IPDL_SOURCES += [
> +    'PMedia.ipdl',
> +]
> +LOCAL_INCLUDES += [

What include files are you using here? Can you make a comment?
Incorporate feedback.
- Non-blocking starting of PBackground.
- More manageable media::Child API thanks to Promise-like MediaPledge helper.
- removed unused code, switched to MOZ_ASSERT, commented on include dependency.

Notes:
- I plan to update the MediaPledge helper to use actual c++ lambda functions.

- Still crashes on shutdown (may always have). Likely a PBackground lifetime issue,
  as MediaManager aggressively tears down its thread (which is used by media::Child
  I presume) in its "xpcom-shutdown" observer [1], which may be too early.
  How do I kill PBackground or delay MediaManager's demise?

Try (doesn't have all nits) - https://treeherder.mozilla.org/#/jobs?repo=try&revision=a69285ed0fbc

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=579c3e8855b4&mark=2013-2013#1989
Flags: needinfo?(bent.mozilla)
Attachment #8580868 - Flags: review?(wmccloskey)
Attachment #8579510 - Attachment is obsolete: true
Covered on irc. I'm not getting BackgroundChild::CloseForCurrentThread() because I'm not an nsThread. See also Bug 1145883 comment 2.
Flags: needinfo?(bent.mozilla)
Now with lambdas!
Attachment #8580868 - Attachment is obsolete: true
Attachment #8580868 - Flags: review?(wmccloskey)
Attachment #8581109 - Flags: review?(wmccloskey)
Comment on attachment 8581109 [details] [diff] [review]
Parts 5-7: enumerateDevices on e10s w/nsICryptoHMAC + clear cookies, lambdas (4) r=keeler, florian

Still no base64 encoding for the mac.
(In reply to Martin Thomson [:mt] from comment #84)
> Still no base64 encoding for the mac.

See comment 78.
Bleargh: that API is terrible...  Is it base64 proper, or just truncation to 7 bits to fit in ascii?
Looks like base64, e.g. 93HlDirf38x7bPw6CU/3EbBPm8tZ2PAvdX+v1olxtRc=
Comment on attachment 8581109 [details] [diff] [review]
Parts 5-7: enumerateDevices on e10s w/nsICryptoHMAC + clear cookies, lambdas (4) r=keeler, florian

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

This looks good. Somebody on the media team should review MediaPledge though.

::: browser/base/content/sanitize.js
@@ +201,5 @@
>  
> +        // Clear deviceIds.
> +        let mediaMgr = Components.classes["@mozilla.org/mediaManagerService;1"]
> +                                 .getService(Ci.nsIMediaManagerService);
> +        mediaMgr.sanitizeDeviceIds(this.range && this.range[0]);

Please comment here that this function does the sanitization asynchronously. So we'll return from here before sanitization is complete. That seems all right to me, but it should be called out.

::: dom/media/MediaManager.cpp
@@ +1427,5 @@
> +  void // NS_IMETHOD
> +  Run()
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +    mozilla::media::SanitizeOriginKeys(mSinceWhen);

My understanding of already_AddRefed is that you'll assert here since you're not using it. Does that not happen? If you want to ignore the promise, I think you can assign it to an nsRefPtr that you don't use or something.

::: dom/media/systemservices/MediaChild.h
@@ +34,5 @@
> +
> +already_AddRefed<ChildPledge<bool>>
> +SanitizeOriginKeys(const uint64_t& aSinceWhen);
> +
> +class Child : public PMediaChild

Just calling this "Child" seems too generic. I realize you're in the media namespace, but it still seems better to call it MediaChild so people know it implements PMediaChild.

::: dom/media/systemservices/PMedia.ipdl
@@ +13,5 @@
> +  manager PBackground;
> +
> +parent:
> +  /**
> +    * Returns a persistent unique secret key for each origin.

The comment is indented over too far. It should be:
/**
 * Blah.
 */

@@ +21,5 @@
> +    */
> +  sync GetOriginKey(nsCString origin, bool privateBrowsing) returns (nsCString key);
> +
> +  /**
> +    * On clear cookies.

Same.

::: dom/media/systemservices/moz.build
@@ +39,5 @@
>  
> +if CONFIG['_MSC_VER']:
> +    DEFINES['__PRETTY_FUNCTION__'] = '__FUNCSIG__'
> +
> +EXPORTS += ['MediaChild.h',

This should be EXPORTS.mozilla.media since we generally try to follow namespace structure.

@@ +50,5 @@
> +]
> +IPDL_SOURCES += [
> +    'PMedia.ipdl',
> +]
> +# /dom/base needed for nsGlobalWindow.h in MediaChild.cpp

Why do you need nsGlobalWindow.h in MediaChild.cpp?

::: ipc/glue/moz.build
@@ +131,5 @@
>  
>  LOCAL_INCLUDES += [
>      '/dom/broadcastchannel',
>      '/dom/indexedDB',
> +    '/dom/media',

Why do you need this?
Attachment #8581109 - Flags: review?(wmccloskey) → review+
Thanks!

(In reply to Bill McCloskey (:billm) from comment #88)
> > +    mozilla::media::SanitizeOriginKeys(mSinceWhen);
> 
> My understanding of already_AddRefed is that you'll assert here since you're
> not using it. Does that not happen?

I never noticed an assert (if it was an NS_ASSERTION I likely would not have noticed). Good catch.

> If you want to ignore the promise, I
> think you can assign it to an nsRefPtr that you don't use or something.

Done.

> ::: dom/media/systemservices/MediaChild.h
> > +already_AddRefed<ChildPledge<bool>>
> > +SanitizeOriginKeys(const uint64_t& aSinceWhen);
> > +
> > +class Child : public PMediaChild
> 
> Just calling this "Child" seems too generic. I realize you're in the media
> namespace, but it still seems better to call it MediaChild so people know it
> implements PMediaChild.

I was actually hoping to reduce some of the name redundancy we have: e.g. media::Child vs. media::MediaChild, especially since this namespace is new and narrowly defined just for this purpose. I see precedence in types like JS::Value. I think this works provided people don't open these namespaces (which, just like with JS, they really shouldn't do). So I would like to keep it if that's ok, and see what jesup thinks.

> > +EXPORTS += ['MediaChild.h',
> 
> This should be EXPORTS.mozilla.media since we generally try to follow
> namespace structure.

So just for disclosure this will put includes in #include "mozilla/media/MediaChild.h" etc. New, but seems good.

> Why do you need nsGlobalWindow.h in MediaChild.cpp?

It pulls in #include "mozilla/MediaManager.h" for the MOZ_ASSERT(MediaManager::IsInMediaThread());

> ::: ipc/glue/moz.build
> @@ +131,5 @@
> >  
> >  LOCAL_INCLUDES += [
> >      '/dom/broadcastchannel',
> >      '/dom/indexedDB',
> > +    '/dom/media',
> 
> Why do you need this?

Removed.
Unbitrot
Attachment #8579473 - Attachment is obsolete: true
Attachment #8582896 - Flags: review+
Incorporate feedback. Carrying forward r=billm, keeler (crypto), r=florian (sanitize.js)
Attachment #8581109 - Attachment is obsolete: true
Attachment #8582899 - Flags: review?(rjesup)
Fix shutdown crashes caused by missing cleanup of BackgroundChild, due to MediaThread not being an nsThread.

Promising green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba76e7b8de4b
Attachment #8582904 - Flags: review?(rjesup)
Or it was for a while. Intermittent shutdown issues.
Some fixes from review were missing in previous try.

Mostly green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=711d014f158e
Comment on attachment 8582899 [details] [diff] [review]
Parts 5-7: enumerateDevices on e10s w/nsICryptoHMAC + clear cookies, lambdas (5) r=keeler, florian, billm

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

r+ with a fair number of nits, but you likely need (trivial) build peer review, and xpcom peer review (froydnj) unless that nsCOMPtr change isn't really needed
Biggest question is over adding (basically) a mozilla::media namespace (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces)

::: dom/media/MediaManager.cpp
@@ +388,5 @@
>      nsCOMPtr<nsIWritableVariant> devices =
>        do_CreateInstance("@mozilla.org/variant;1");
>  
> +    size_t len = mDevices->Length();
> +    if (!len) {

nit: prefer == 0 for non-pointers

@@ +1508,5 @@
> +          media::GetOriginKey(mOrigin, mInPrivateBrowsing);
> +      p->Then([runnable](nsCString result) mutable {
> +        runnable->mOriginKey = result;
> +        NS_DispatchToMainThread(runnable);
> +      }, [](nsresult rv){});

Is this (new?) syntax generally available?
Add a comment about what this is doing

::: dom/media/VideoUtils.cpp
@@ +291,5 @@
> +nsresult
> +GenerateRandomPathName(nsCString& aOutSalt, uint32_t aLength)
> +{
> +  nsresult rv = GenerateRandomName(aOutSalt, aLength);
> +  if (NS_FAILED(rv)) return rv;

ok, but only because the rest of the file does this ;-)

::: dom/media/systemservices/MediaChild.cpp
@@ +14,5 @@
> +
> +#undef LOG
> +#define LOG(args) PR_LOG(gMediaChildLog, PR_LOG_DEBUG, args)
> +
> +PRLogModuleInfo *gMediaChildLog;

Should any of this be ifdef PR_LOGGING?

@@ +46,5 @@
> +
> +template<typename ValueType> NS_IMPL_ADDREF(ChildPledge<ValueType>)
> +template<typename ValueType> NS_IMPL_RELEASE(ChildPledge<ValueType>)
> +template<typename ValueType> NS_INTERFACE_MAP_BEGIN(ChildPledge<ValueType>)
> +  NS_INTERFACE_MAP_ENTRY(nsIIPCBackgroundChildCreateCallback)

indent wrong

@@ +105,5 @@
> +Child::Child()
> +{
> +#if defined(PR_LOGGING)
> +  if (!gMediaChildLog)
> +    gMediaChildLog = PR_NewLogModule("MediaChild");

braces

::: dom/media/systemservices/MediaChild.h
@@ +13,5 @@
> +#include "nsIIPCBackgroundChildCreateCallback.h"
> +#include "MediaUtils.h"
> +
> +namespace mozilla {
> +namespace media {

We're not supposed to be adding new sub-namespaces to mozilla.  Check the style guidelines.  (Yes, I know TimeUnits.h has "namespace media", and there's one bit in the windows video capture code)
Is there a good reason for this?

@@ +22,5 @@
> +{
> +  NS_DECL_NSIIPCBACKGROUNDCHILDCREATECALLBACK
> +  NS_DECL_ISUPPORTS
> +public:
> +  explicit ChildPledge();

Comments?  All the MediaChild/Parent stuff is totally uncommented....  please explain what it's there for and what it's expected to do

::: dom/media/systemservices/MediaParent.cpp
@@ +17,5 @@
> +#include "nsAppDirectoryServiceDefs.h"
> +#include "nsISupportsImpl.h"
> +#include "prlog.h"
> +
> +PRLogModuleInfo *gMediaParentLog;

move inside ifdef PR_LOGGING?

@@ +338,5 @@
> +  {
> +    if (!sMediaParentSingleton) {
> +      sMediaParentSingleton = new MediaParentSingleton();
> +    }
> +    return sMediaParentSingleton;

Is this only ever called from one thread?  At least mark this as non-threadsafe; extra points if you can add debug-only assertion of that.
Also, I presume the callers of ::Get() unwind their refs before shutdown so this will get cleaned up.

::: dom/media/systemservices/MediaParent.h
@@ +12,5 @@
> +
> +#include "MediaChild.h"
> +
> +namespace mozilla {
> +namespace media {

ditto

::: dom/media/systemservices/MediaUtils.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/unused.h"
> +#include "nsThreadUtils.h"

this should be including MediaUtils.h first (and that means getting the includes in MediaUtils right)

::: dom/media/systemservices/MediaUtils.h
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_MediaUtils_h
> +#define mozilla_MediaUtils_h
> +

Missing includes.... makes assumption about what's been included before this

@@ +10,5 @@
> +namespace mozilla {
> +namespace media {
> +
> +template<typename ValueType>
> +class Pledge

Describe what a Pledge is and why

::: dom/media/systemservices/moz.build
@@ +37,5 @@
>          ]
>      ]
>  
> +if CONFIG['_MSC_VER']:
> +    DEFINES['__PRETTY_FUNCTION__'] = '__FUNCSIG__'

Need build peer review

::: xpcom/glue/nsCOMPtr.h
@@ +399,5 @@
>      if (mRawPtr) {
>        nsCOMPtr<T> query_result(do_QueryInterface(mRawPtr));
> +      if (query_result.get() != mRawPtr) {
> +        NS_ASSERTION(query_result.get() == mRawPtr, "QueryInterface needed");
> +      }

Why this change?
Touching this will need xpcom peer review
Attachment #8582899 - Flags: review?(rjesup) → review+
Comment on attachment 8582904 [details] [diff] [review]
Part 8: cleanup MediaThread's BackgroundChild on shutdown

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

::: dom/media/MediaManager.cpp
@@ +2218,5 @@
> +      }
> +      nsRefPtr<nsRunnable> mReply;
> +    };
> +
> +    MediaManager::GetMessageLoop()->PostTask(FROM_HERE, new ShutdownTask(

Is MediaManager:: needed here?
Note: will this block MainThread with a nested event loop here?  At minimum comment it.  Blocking Observe() (or interrupting it with a nested event loop, if that's happening here) may be problematic; please ping froyd on this if so.

If it doesn't block it, we just have to be ok with deferring the actually shutdown and clearing of things.

@@ +2225,1 @@
>        MutexAutoLock lock(mMutex);

indent

@@ +2225,4 @@
>        MutexAutoLock lock(mMutex);
>        GetActiveWindows()->Clear();
>        mActiveCallbacks.Clear();
>        mCallIds.Clear();

I think this should be done before posting the shutdown task

::: dom/media/systemservices/MediaUtils.h
@@ +89,5 @@
>  };
>  
> +// General purpose runnable with an eye toward lambdas
> +
> +namespace CallbackRunnable

Ping ehsan and/or froydnj or m.d.platform about this namespace
(In reply to Randell Jesup [:jesup] from comment #98)
> r+ with a fair number of nits, but you likely need (trivial) build peer
> review, and xpcom peer review (froydnj) unless that nsCOMPtr change isn't
> really needed

Just debug crud I accidentally left behind, sorry. Will remove. I wont be touching nsCOMPtr.

> Biggest question is over adding (basically) a mozilla::media namespace
> (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Namespaces)

Will check with Ehsan.

> > +          media::GetOriginKey(mOrigin, mInPrivateBrowsing);
> > +      p->Then([runnable](nsCString result) mutable {
> > +        runnable->mOriginKey = result;
> > +        NS_DispatchToMainThread(runnable);
> > +      }, [](nsresult rv){});
> 
> Is this (new?) syntax generally available?

Yes C++11 lambdas are green on try and mozilla-approved according to
https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

I think this is a promising pattern for e10s proxying, no pun intended.

> > +namespace mozilla {
> > +namespace media {
> 
> We're not supposed to be adding new sub-namespaces to mozilla.  Check the
> style guidelines.  (Yes, I know TimeUnits.h has "namespace media", and
> there's one bit in the windows video capture code)
> Is there a good reason for this?

I followed the pattern gcp used in his patch I cribbed in Bug 1104616, but I like it, as it lets us collect related things without having them be in a single class. I connected with ehsan on irc who said he's given up enforcing this, and that he thinks it's ok as long as we're aware of the implications, which is pretty much limited to indiscriminate and frivolous use of "using namespace" use at the top level in unified sources, something we're better off policing the use of IMHO.

My intent is for people to use media::Child instead of MediaChild, rather than try to open the namespace, just like you'd use JS::Value and never open the JS namespace. Also, note that I forgot to rename MediaParent to media::Parent... the same way for symmetry. Will fix in next patch FYI.

> > +  {
> > +    if (!sMediaParentSingleton) {
> > +      sMediaParentSingleton = new MediaParentSingleton();
> > +    }
> > +    return sMediaParentSingleton;
> 
> Is this only ever called from one thread?

Good catch. No, there may be more than one Background thread in the master process. I've seen one master thread to service the child in the content process (from GetOriginKey) and another master thread to service the child in the master process (from SanitizeOriginKeys). That's why I needed the singleton, but obviously I'll need locking unless locking is done for me somehow, which it doesn't seem like it is.

> Also, I presume the callers of ::Get() unwind their refs before shutdown so
> this will get cleaned up.

I presume so too or we would see leaks.

> ::: dom/media/systemservices/moz.build
> @@ +37,5 @@
> >          ]
> >      ]
> >  
> > +if CONFIG['_MSC_VER']:
> > +    DEFINES['__PRETTY_FUNCTION__'] = '__FUNCSIG__'
> 
> Need build peer review

Removed.

> ::: xpcom/glue/nsCOMPtr.h
> @@ +399,5 @@
> >      if (mRawPtr) {
> >        nsCOMPtr<T> query_result(do_QueryInterface(mRawPtr));
> > +      if (query_result.get() != mRawPtr) {
> > +        NS_ASSERTION(query_result.get() == mRawPtr, "QueryInterface needed");
> > +      }
> 
> Why this change?
> Touching this will need xpcom peer review

Removed.
(In reply to Randell Jesup [:jesup] from comment #99)
> > +    MediaManager::GetMessageLoop()->PostTask(FROM_HERE, new ShutdownTask(
> 
> Is MediaManager:: needed here?

Probably not, but that's the pattern used in this file.

> Note: will this block MainThread with a nested event loop here?

No.

> At minimum comment it.

Here's what I've put in:

    // Post ShutdownTask to execute on mMediaThread and pass in a lambda
    // callback to be executed back on this thread once it is done.
    //
    // The lambda callback "captures" the 'this' pointer for member access.
    // This is safe since this is guaranteed to be here since sSingleton isn't
    // cleared until the lambda function clears it.

> If it doesn't block it, we just have to be ok with deferring the actually
> shutdown and clearing of things.

Yes, are we ok?

> @@ +2225,1 @@
> >        MutexAutoLock lock(mMutex);
> 
> indent

I think the indent is correct here since the first two lines is one long line word-wrapped by 4 spaces.

    MediaManager::GetMessageLoop()->PostTask(FROM_HERE, new ShutdownTask(
        media::CallbackRunnable::New([this]() mutable {
      // Close off any remaining active windows.
      MutexAutoLock lock(mMutex);

lambdas are inlined c++ functions, much like in JS. Our style guide doesn't cover this, so I used our JS style rules.

> @@ +2225,4 @@
> >        MutexAutoLock lock(mMutex);
> >        GetActiveWindows()->Clear();
> >        mActiveCallbacks.Clear();
> >        mCallIds.Clear();
> 
> I think this should be done before posting the shutdown task

The old code Stop()ed the mMediaThread inside the same mutex, so that may no longer be safe. What happens if mMediaThread services other events before servicing ShutdownTask? It might add windows and callbacks and callids so these would no longer be clear on shutdown. I've conservatively tried to protect the old order of things.
 
> > +namespace CallbackRunnable
> 
> Ping ehsan and/or froydnj or m.d.platform about this namespace

Ehsan seemed resigned to let this fly. I could have used a static class, but if the only problem is people using "using namespace" statements then there's no real problem here I claim. I use a combination of namespace and New() creator-function to work around c++ only having argument resolution for template functions and not template classes.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #101)
> Ehsan seemed resigned to let this fly.

I should clarify: we had a general discussion about namespaces and their problems, not specifically looking at CallbackRunnable.
Incorporate feedback. Carrying forward lots of reviews. Re-review for StaticMutex.
Attachment #8582899 - Attachment is obsolete: true
Attachment #8583361 - Flags: review?(rjesup)
Incorporate feedback, removing things that would need additional reviewers.

Always good to re-try when adding a mutex:

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac711b560202
Attachment #8583363 - Flags: review?(rjesup)
Attachment #8582904 - Attachment is obsolete: true
Attachment #8582904 - Flags: review?(rjesup)
> > If it doesn't block it, we just have to be ok with deferring the actually
> > shutdown and clearing of things.

Ok.  I presume we're blocked from sending a second Shutdown by the fact that we're doing this from xpcom-shutdown, and removing the observer.

So: why do we need to defer the rest of the cleanup until the task stops?  All that needs to be done after the task is shutdown is call Stop, correct?  What we do need to do is block new requests in shutdown (and perhaps assert on them), and silently fail any attempt to use the MessageLoop.  However, these are existing issues; you just need to post the shutdown message and defer Stop until it's done for your patch unless this is causing new oranges (and maybe it is).  The singleton itself doesn't need to survive until then; the pointer to the Thread can live in the lambda.

> Yes, are we ok?

If you're green on Try, I'm ok with it; we can clean up as part of greater MediaManager cleanup.
 
> I think the indent is correct here since the first two lines is one long
> line word-wrapped by 4 spaces.

Ok, wrap it please

> > @@ +2225,4 @@
> > >        MutexAutoLock lock(mMutex);
> > >        GetActiveWindows()->Clear();
> > >        mActiveCallbacks.Clear();
> > >        mCallIds.Clear();
> > 
> > I think this should be done before posting the shutdown task
> 
> The old code Stop()ed the mMediaThread inside the same mutex, so that may no
> longer be safe. What happens if mMediaThread services other events before
> servicing ShutdownTask? It might add windows and callbacks and callids so
> these would no longer be clear on shutdown. I've conservatively tried to
> protect the old order of things.

Understood.  Per above can be dealt with later.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #103)
> Created attachment 8583361 [details] [diff] [review]
> Parts 5-7: enumerateDevices on e10s w/nsICryptoHMAC + clear cookies, lambdas
> (6) r=keeler, florian, billm
> 
> Incorporate feedback. Carrying forward lots of reviews. Re-review for
> StaticMutex.

There's no StaticMutex in this patch....
Comment on attachment 8583363 [details] [diff] [review]
Part 8: cleanup MediaThread's BackgroundChild on shutdown (2)

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

r- for now because I believe this patchset is doing blocking IO on PBackground's single CHrome reception thread, and I think that's a no-no.  Billm is looking at it; if this assumption is wrong, then r+

::: dom/media/systemservices/MediaParent.cpp
@@ +337,5 @@
> +    // Protect creation of singleton and access from multiple Background threads.
> +    //
> +    // Multiple Background threads happen because sanitize.js calls us from the
> +    // chrome process and gets a thread separate from the one servicing ipc from
> +    // the content process.

You mean, sanitize.js calls us from MainThread in the Chrome process, while ipc is handled by the PBackground IPC thread in Chrome.  As written, it's confusing due to the "from the chrome process" bit.

Question: why is this PBackground - to avoid IO on MainThread I presume?  Is it allowed (perf-wise) to do file IO on the PBackground thread handling IO from the Content process?
Attachment #8583363 - Flags: review?(rjesup) → review-
Comment on attachment 8583361 [details] [diff] [review]
Parts 5-7: enumerateDevices on e10s w/nsICryptoHMAC + clear cookies, lambdas (6) r=keeler, florian, billm

(In reply to Randell Jesup [:jesup] from comment #106)
> There's no StaticMutex in this patch....

Whoops, looks like I put it in Part 8 by mistake. That's just as well, then I can forward r+ on this one.
Attachment #8583361 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #107)
> You mean, sanitize.js calls us from MainThread in the Chrome process, while
> ipc is handled by the PBackground IPC thread in Chrome.  As written, it's
> confusing due to the "from the chrome process" bit.

Technically sanitize.js calls through MediaManager which dispatches to mMediaThread, which calls media::Child.SanitizeOriginKeys(). That way no locking is needed in media::Child. I was hoping to deal with the unnecessary opening of MediaManager in a follow-up.

> Question: why is this PBackground - to avoid IO on MainThread I presume?

Yes.

> Is it allowed (perf-wise) to do file IO on the PBackground thread handling IO from the Content process?

Looks like the answer from our irc conversation is: no.

I'll look at dispatching the actual file IO to STSThread.
Move blocking file IO to the Stream Transport thread.
Attachment #8583363 - Attachment is obsolete: true
Attachment #8584952 - Flags: review?(rjesup)
Comment on attachment 8584952 [details] [diff] [review]
Part 8: IO on STS thread + cleanup BackgroundChild on shutdown (3)

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

::: dom/media/systemservices/MediaChild.cpp
@@ +118,5 @@
>    sChild = nullptr;
>    MOZ_COUNT_DTOR(Child);
>  }
>  
> +int Child::sRequestCounter = 0;

Is there a reason for this to be int instead of a concrete size like uint32/64_t?  I realize it uses ==, so if int wraps around it should be negative and work, but I'm not sure that c++ actually defines what happens on an overflow.
Either a concrete size, or "unsigned int"

@@ +160,3 @@
>  {
> +  Child* obj = new Child();
> +  obj->AddRef();

Should this be returning an already_AddRefed<>?  Perhaps that's not the normal pattern for IPC allocators, so it's not a big deal.
Attachment #8584952 - Attachment is obsolete: false
Comment on attachment 8585023 [details] [diff] [review]
Part 8: IO on STS thread + cleanup BackgroundChild on shutdown (4)

carry forward
Attachment #8585023 - Flags: review?(rjesup) → review+
Attachment #8584952 - Attachment is obsolete: true
(In reply to Randell Jesup [:jesup] from comment #113)
> > +int Child::sRequestCounter = 0;
> 
> Is there a reason for this to be int instead of a concrete size like
> uint32/64_t?  I realize it uses ==, so if int wraps around it should be
> negative and work, but I'm not sure that c++ actually defines what happens
> on an overflow.
> Either a concrete size, or "unsigned int"

It shouldn't matter, I just prefer plain types, but I'm happy to make it uint32_t.

> >  {
> > +  Child* obj = new Child();
> > +  obj->AddRef();
> 
> Should this be returning an already_AddRefed<>?  Perhaps that's not the
> normal pattern for IPC allocators, so it's not a big deal.

No, IPDL manages lifetimes for its protocols, and does so without reference counting. I'm trying to follow this pattern [1] since I rely on reference-counting the Parent object in at least one place, and it misbehaved badly when the IPDL manager's owning reference wasn't accounted for (Use-after-free).

Just for my own sanity as well, I think this is the safest pattern for maintenance since reference counting is so common on our tree and suddenly wanting it and not doing it right because it mixes poorly with the IPDL manager, is a common trap (that I just fell into).

[1] https://developer.mozilla.org/en-US/docs/IPDL/Best_Practices#Reference_counting_protocol_actors_is_tricky
Incorporate feedback. Carrying forward r=jesup.
Attachment #8585023 - Attachment is obsolete: true
Attachment #8585042 - Flags: review+
Unblunder. Now builds.
Attachment #8585042 - Attachment is obsolete: true
Attachment #8585100 - Flags: review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/99c3baae746d. MSVC says it's never heard of the idea of producing a __PRETTY_FUNCTION__ (https://treeherder.mozilla.org/logviewer.html#?job_id=8203696&repo=mozilla-inbound) and mxr seems to say that if you're desperate for it in cross-platform code you do something like http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginMessageUtils.h#57 but mostly you just don't.
And on Android debug, a charmingly Inception-like https://treeherder.mozilla.org/logviewer.html#?job_id=8205310&repo=mozilla-inbound crashing and then crashing the crashreporter.
Sorry for the drive-by, but I happened to notice that the parent process is blindly trusting any origin sent from the child process via the GetOriginKey message. We shouldn't trust anything the child sends us! A hacked child process can put anything in the messages it sends, or send messages in a funny order, etc.

Usually origins are verified by having the child send a nsIPrincipal/PrincipalInfo with the message, and then we ask the ContentParent if it is conceivable for the child process to own the given principal (via the AssertAppPrincipal function).
relanded (with just PRETTY_FUNCTION changes, since I noted the IRC and not the bug), and re-backed-out once I realized there were test failures too

https://hg.mozilla.org/integration/mozilla-inbound/rev/826a68078904
(In reply to Phil Ringnalda (:philor) from comment #121)
> __PRETTY_FUNCTION__

So sorry, I had fixed this already in comment 112, but in a new Patch 5-7 that I forgot to upload this bug. :-(

(In reply to Phil Ringnalda (:philor) from comment #123)
> And on Android debug, a charmingly Inception-like

This one too (wrong PR_Init global in MediaChild).

(In reply to Phil Ringnalda (:philor) from comment #124)
> And xpcshell since it clears history,

And this one. Stacktrace points right to it.

I'll reupload all patches.
(In reply to Phil Ringnalda (:philor) from comment #126)
> One last possibly-different crash, debug b2g emulator,
> https://treeherder.mozilla.org/logviewer.html#?job_id=8205805&repo=mozilla-
> inbound

This one I can't as easily account for since, while this patch does start its new code out in the same GetUserMediaDevicesTask as the one used by the gUM selection dialog, any new code is effectively if'ed out in the else of if (mPrivileged). In other words: We're not anonymizing device-ids returned to the chrome JS code implementing the device selector UI in the gUM doorhanger.

Are we sure this is not an intermittent? I see a:

> 20:31:02 INFO - XXX FIXME : Got a mozContentEvent: permission-deny 

right before in the log, as well as signs of things going haywire with messaging right before during shutdown. Or do we see these on good runs as well, do you know? Any information appreciated.
Flags: needinfo?(philringnalda)
(In reply to Ben Turner <unresponsive until 3/30> (use the needinfo flag!) from comment #125)
> Sorry for the drive-by, but I happened to notice that the parent process is
> blindly trusting any origin sent from the child process via the GetOriginKey
> message. We shouldn't trust anything the child sends us! A hacked child
> process can put anything in the messages it sends, or send messages in a
> funny order, etc.

The "secret key" returned to the child is used by chrome (c++) code to make enumerateDevices return unique-per-origin device-ids rather than supercookies. The origin (through the key) is an input to the uniqueifyer.

If a Child process is hacked to the point where it is able to call this method, then merely using its own origin would give it all it needs to learn the underlying device ids.

Naively, I don't see how learning the keys to decipher the underlying device ids from other origins helps an attacker when presumably they could run the same attack from whichever origin they control, and for origins they don't control, having this key seems of little value.

If these assumptions are wrong (which is quite possible), please take this as a request to learn. :)

> Usually origins are verified by having the child send a
> nsIPrincipal/PrincipalInfo with the message, and then we ask the
> ContentParent if it is conceivable for the child process to own the given
> principal (via the AssertAppPrincipal function).

Thanks for the info. Is there an example I can look at? But primarily please let me know if you still think this is a blocker for landing, otherwise would filing a follow-up be an option?
Flags: needinfo?(bent.mozilla)
Re-upload as bitrot precaution.
Attachment #8582896 - Attachment is obsolete: true
Attachment #8582897 - Attachment is obsolete: true
Attachment #8585167 - Flags: review+
Unbitrot and missing fixes that caused trouble discussed. Carrying forward reviews.
- s/__PRETTY_FUNCTION__/__FUNCTION__/
- Fixed wrong gMediaChildLog.
Attachment #8583361 - Attachment is obsolete: true
Attachment #8585168 - Flags: review+
I also forgot, here’s a completed additional precautionary try I did yesterday that I think had everything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b39398021e
(In reply to Jan-Ivar Bruaroey [:jib] from comment #137)
> completed additional precautionary try I did yesterday that I think had everything:

Minus nit fixes (a whitespace + int to uint32_t).
(In reply to Jan-Ivar Bruaroey [:jib] from comment #130)
> The "secret key" returned to the child is used by chrome (c++) code to make
> enumerateDevices return unique-per-origin device-ids rather than
> supercookies.

Ok, so the uniquification isn't important for security, rather just for privacy?

> having this key seems of little value.

Yeah, it sounds like it... 

> Thanks for the info. Is there an example I can look at?

I'd just look for code that calls AssertAppPrincipal (e.g. CheckPrincipalWithCallbackRunnable is an attempt to make it simpler from PBackground).

> But primarily please
> let me know if you still think this is a blocker for landing, otherwise
> would filing a follow-up be an option?

It sounds like a followup to investigate this is fine. Let's make sure the keys are really not important before moving on too far.
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner <unresponsive until 3/30> (use the needinfo flag!) from comment #139)
> Ok, so the uniquification isn't important for security, rather just for
> privacy?

Right, fingerprinting.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #136)
> Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=446d7b17ba84

Hit a MOZ_CRASH(ParentSingleton not thread-safe) in
https://treeherder.mozilla.org/logviewer.html#?job_id=6049656&repo=try

Please hold off.
s/NS_DECL_ISUPPORTS/NS_DECL_THREADSAFE_ISUPPORTS/ for ParentSingleton to fix comment 141.

Carrying forward reviews.

Re-try win7 bc1 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f1c3c3cf635
Attachment #8585169 - Attachment is obsolete: true
Attachment #8585193 - Flags: review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #129)
> Are we sure this is not an intermittent? I see a:

It's b2g debug, so absolutely anything is possible, but since we saw the exact same thing on both landings on inbound, and haven't seen it in any other runs, including your current try run, I think by far the most likely thing is that it was just b2g's expression of the same bustage that in desktop was the sanitize crash.
Flags: needinfo?(philringnalda)
I think I've licked it. I failed to adhere to our "wet our pants rather than get off the bus" strategy to main-only objects on media thread.

Try b2g debug m1 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46a888b7a4a
Attachment #8585193 - Attachment is obsolete: true
Attachment #8585200 - Flags: review?(rjesup)
As the last two try links demonstrate, I've tried and failed to use try server conservatively and do so correctly. Here's a new try all:

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b34fe66e143

Randell, feel free to land if the latest change passes your review. Thanks!
Flags: needinfo?(rjesup)
Try looks green (except for infra OSX 10.8 opt)! Pity inbound is closed.
Attachment #8585200 - Flags: review?(rjesup) → review+
Flags: needinfo?(rjesup)
Depends on: 1157995
Depends on: 1162720
Depends on: 1169665
Blocks: 1178366
Depends on: 1233691
Depends on: 1399922
You need to log in before you can comment on or make changes to this bug.