Closed Bug 1267918 Opened 7 years ago Closed 7 years ago

Ensure GMP crash reports are being reported for unencrypted GMP decoding

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 8 obsolete files)

Testcase; transplanting to another window
716 bytes, text/html
Details
Bug 1267918 - Split LoadGMP message into select and load messages.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1267918 - Add GMPCrashHelper to GMPService::GetGMP* functions.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1267918 - Add GMPCrashHelperHolder to automatically disconnect crash helpers on actor destory.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1267918 - Add GMPCrashHelper for MediaKeys.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1267918 - Add GMPCrashHelper for HTMLMediaElement.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1267918 - Add GMPCrashHelper for WebAudio buffer decoder.
58 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1267918 - Remove obsolete GMP crash handling code.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1267918 - Add do_AddRef(const RefPtr<T>& aObj).
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1267918 - Ensure GMPCrashHelper instances are destroyed on the main thread.
58 bytes, text/x-review-board-request
karlt
: review+
Details 
Split off from bug 1266286; GMP crash reports are not being collected when unencrypted GMP decoding is being used.

STR (Windows):
1. Turn off WMF, via media.wmf.enabled=false.
2. Open http://pearce.org.nz/h
3. Video will play.
4. Open about config and toggle media.gmp.plugin.crash=true. You'll need to create this pref the first time.
5. Observe that the video errors out, but there's no crash report dialog drop down from chrome.

The problem is that we're not calling GeckoMediaPluginService::AddPluginCrashedEventTarget() with the plugin ID of the GMP we're using, and the window to which the PluginCrashedEvent needs to be dispatched to.

In the EME/Encrypted case, we do that in MediaKeys::OnCDMCreated().

In the unencrypted case, we're running the GMP in the GMP{Audio,Video}Decoder object off-main-thread, and we don't have easy access to the window object (it's several layers up, in the HTMLMediaElement on the main thread), and we don't have the GMP's ID number either.

So we'll need to expose the GMP ID number; probably we can just add an accessor to it to GMP{Audio,Video}DecoderProxy.

Note: we had to disable unencrypted decoding using the Adobe GMP because of unreported crashes. So fixing this blocks turning MP4 decoding via the Adobe GMP on WinXP back on.

Getting the window pointer will be harder; it, or a token representing it, will need to be piped through from the HTMLMediaElement somehow.
Extract all the parameters passed to CreateAudioDecoder/CreateVideoDecoder and place them into a structure that is passed down to the creation of the actually decoder, where the relevant parameters can be extracted.

This will be used in a future patch to pass the window id of the inner window to GMP-based decoders, alleviating the need to .

MozReview-Commit-ID: 9LZlcfRVz6A
MozReview-Commit-ID: DY7UCYfOb25
MozReview-Commit-ID: 1Q90g3ehkXf
MozReview-Commit-ID: 1n4MdfoQtDY
MozReview-Commit-ID: UICBFgDHBm
MozReview-Commit-ID: 7UXY5uEQUtj
MozReview-Commit-ID: GYQTbWPlsBI
Attached patch P8: WIP (obsolete) — — Splinter Review 
MozReview-Commit-ID: LJSHqWvgq8i
Chris, I've attached my WIP changes for threading the window id through the GMP. The plan was to reverse the window id into a WindowOuter for registration.

The major part is to implement the ipdl bridge from PGMPContentParent to PGMPContentChild.
Flags: needinfo?(cpearce)
Testcase; moving video elements between iframes and windows.

With the current set of patches, we assume that the window ID doesn't change for the lifetime of the video element. This testcase shows that's a bad assumption.

This testcase shows that the WindowID of the window containing the video element changes when you transplant the video into another iframe/document (as you'd expect). And if you transplant the video element to a new browser window (i.e. tree of documents), the WindowID corresponds to a window in the original doctree, and the crash report UI ends up getting shown on the original browser window, not the window that contains the video after the transplant.
Flags: needinfo?(cpearce)
Assignee: dglastonbury → cpearce
Attachment #8760999 - Attachment is obsolete: true
Attachment #8761000 - Attachment is obsolete: true
Attachment #8761002 - Attachment is obsolete: true
Attachment #8761003 - Attachment is obsolete: true
Attachment #8761004 - Attachment is obsolete: true
Attachment #8761005 - Attachment is obsolete: true
Attachment #8761006 - Attachment is obsolete: true
Attachment #8761007 - Attachment is obsolete: true
This will allow us to attach a crash handler to a GMP process after deciding
which GMP to load but before actually loading it.

Review commit: https://reviewboard.mozilla.org/r/61142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61142/
Attachment #8766117 - Flags: review?(gsquelart)
Attachment #8766118 - Flags: review?(gsquelart)
Attachment #8766119 - Flags: review?(gsquelart)
Attachment #8766120 - Flags: review?(gsquelart)
Attachment #8766121 - Flags: review?(gsquelart)
Attachment #8766122 - Flags: review?(karlt)
Attachment #8766123 - Flags: review?(gsquelart)
This enables callers to specify a way to determine the correct window to
dispatch the PluginCrashed event to should the GMP actor crash.

We need a way to determine the correct window at crash time, as the GMP's
window can change at runtime. For example, if the GMP is being used for
unencrypted decoding, the <video> element can be moved to a new browser window
at runtime.

Note: I don't handle disconnecting the GMPCrashHandlers in this patch; we do
delete the GMPCrashHandlers in this patch when their associated GMP crashes, and
in the next patch we handle disconnecting GMPCrashHandlers in the case where
we don't crash.

Review commit: https://reviewboard.mozilla.org/r/61144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61144/
Disconnecting the GMPCrashHelpers at the right time is hard, because in the
crashing case we're all shutdown before the GMPCrashHelpers can be invoked to
help handle the crash report. So add a class to help the helpers;
GMPCrashHelperHolder. This composes into the GMP content protocol actors, to
help them disconnect the crash helpers at the right time. See the comment in
GMPCrashHelperHolder for the details.

Review commit: https://reviewboard.mozilla.org/r/61146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61146/
So if a GMP crashes while doing EME, we'll get a crash report using the new
mechanism.

Review commit: https://reviewboard.mozilla.org/r/61148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61148/
This ensures that unencrypted GMP decoding crash reporting works.

Review commit: https://reviewboard.mozilla.org/r/61150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61150/
This means if WebAudio is using the Adobe GMP for decoding and it crashes,
we'll get a crash report for the GMP.

Review commit: https://reviewboard.mozilla.org/r/61152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61152/
Comment on attachment 8766117 [details]
Bug 1267918 - Split LoadGMP message into select and load messages.

https://reviewboard.mozilla.org/r/61142/#review58014

r+ with the one issue addressed. Non-issues are up to you.

::: dom/media/gmp/GMPServiceParent.cpp:1827
(Diff revision 1)
> +GeckoMediaPluginServiceParent::GetById(uint32_t aPluginId)
> +{
> +  MutexAutoLock lock(mMutex);
> +  for (RefPtr<GMPParent>& gmp : mPlugins) {
> +    if (gmp->GetPluginId() == aPluginId) {
> +      return RefPtr<GMPParent>(gmp).forget();

I think you could use |return do_AddRef(gmp)|.
(It does exactly the same thing, but I think it's a bit more self-explanatory.)

::: dom/media/gmp/GMPServiceParent.cpp:1846
(Diff revision 1)
> -                              ProcessId* aId,
> +                                nsresult* aOutRv)
> -                              nsCString* aDisplayName,
> -                              uint32_t* aPluginId,
> -                              nsresult* aRv)
>  {
> -  *aRv = NS_OK;
> +  *aOutRv = NS_OK;

You don't really need this one, as you overwrite aOutRv a line 1848 or 1853 anyway.
(Depending on how you change the code below, of course; see next issue.)

::: dom/media/gmp/GMPServiceParent.cpp:1853
(Diff revision 1)
> +  *aOutRv = gmp ? NS_OK : NS_ERROR_FAILURE;
> +  *aOutPluginId = gmp->GetPluginId();

1\. aOutRv is already initialized to 'NS_OK', there's no need to overwrite it when gmp\!=null (unless you've removed the first initialization as suggested above),
2\. If gmp is null, gmp->GetPluginId() will probably crash, or worse.

So I think you should just go with an if/else statement instead, e.g.:
  if (gmp) \{ \*aOutPluginId = ...; \} else \{ \*aOutRv = NS_ERROR_FAILURE; \}

::: dom/media/gmp/GMPServiceParent.cpp:1865
(Diff revision 1)
> +}
> +
> +bool
> +GMPServiceParent::RecvLaunchGMP(const uint32_t& aPluginId,
> +                                nsTArray<ProcessId>&& aAlreadyBridgedTo,
> +                                base::ProcessId* aOutProcessId,

I don't think you need 'base::' here.

::: dom/media/gmp/GMPServiceParent.cpp:1881
(Diff revision 1)
> +  if (!gmp) {
> +    *aOutRv = NS_ERROR_FAILURE;
> +    return true;
> +  }
> +
> +  if (!gmp || !gmp->EnsureProcessLoaded(aOutProcessId)) {

You cannot be in the '!gmp' case at this point (because of the return above), so you can remove it from this test.
Attachment #8766117 - Flags: review?(gsquelart) → review+
Attachment #8766122 - Flags: review?(karlt) → review-
Comment on attachment 8766122 [details]
Bug 1267918 - Add GMPCrashHelper for WebAudio buffer decoder.

https://reviewboard.mozilla.org/r/61152/#review58034

::: dom/media/webaudio/BufferDecoder.cpp:27
(Diff revision 1)
>    MOZ_COUNT_CTOR(BufferDecoder);
>  }
>  
>  BufferDecoder::~BufferDecoder()
>  {
>    // The dtor may run on any thread, we cannot be sure.

I guess this is still true.

That means mCrashHelper's nsWeakPtr could be destroyed on a non-main thread.

I expect that would be bad, if it is non-null, because nsWeakReference doesn't have thread-safe ref-counting.

::: dom/media/webaudio/BufferDecoder.cpp:75
(Diff revision 1)
>  }
>  
> +GMPCrashHelper*
> +BufferDecoder::GetCrashHelper()
> +{
> +  return mCrashHelper;

I wonder whether creating single-use BufferDecoderGMPCrashHelper here may be an option, to save an allocation for what is usually
unused.

That's not worth losing sleep over.

But perhaps moving the nsWeakPtr to the BufferDecoder might make it easier to clear the nsWeakPtr at an appropriate time.

::: dom/media/webaudio/MediaBufferDecoder.cpp:192
(Diff revision 1)
> +  explicit BufferDecoderGMPCrashHelper(nsPIDOMWindowInner* aParent)
> +    : mParent(do_GetWeakReference(aParent))
> +  {
> +  }
> +  already_AddRefed<nsPIDOMWindowInner> GetPluginCrashedEventTarget() override {
> +    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryReferent(mParent);

MOZ_ASSERT(NS_IsMainThread()) please.

Similarly in the destructor.
Comment on attachment 8766118 [details]
Bug 1267918 - Add GMPCrashHelper to GMPService::GetGMP* functions.

https://reviewboard.mozilla.org/r/61144/#review58044

r+ with 1 issue, and 2 optional nits:

::: dom/media/gmp/GMPService.h:30
(Diff revision 1)
>  template <class> struct already_AddRefed;
>  
> +// For every GMP actor requested, the caller can specify a crash helper,
> +// which is an object which supplies the nsPIDOMWindowInner to which we'll
> +// dispatch the PluginCrashed event if the GMP crashes.
> +class GMPCrashHelper {

Style: '{' on separate line.

::: dom/media/gmp/GMPService.cpp:303
(Diff revision 1)
> +    nsCOMPtr<nsIDocument> document(window->GetExtantDoc());
> +    if (NS_WARN_IF(!document)) {
> +      continue;
> +    }

You can move lines 303-306 ('document' definition and check) before line 295, to avoid the unnecessary work on 'init'.

::: dom/media/gmp/GMPService.cpp:630
(Diff revision 1)
> +  if (!mPluginCrashHelpers.Get(aPluginId, &helpers)) {
> +    helpers = new nsTArray<RefPtr<GMPCrashHelper>>();
> +    mPluginCrashHelpers.Put(aPluginId, helpers);
> +  }
> +  if (!helpers->Contains(aHelper)) {
> +    helpers->AppendElement(aHelper);
> +  }

You don't need to test helper->Contains when there's no helper list yet, e.g.:
```
if (!mPluginCrashHelpers.Get(...)) {
  ...
} else if (helpers->Contains(aHelper)) {
  return;
}
helpers->AppendElement(aHelper);
```
Attachment #8766118 - Flags: review?(gsquelart) → review+
Attachment #8766119 - Flags: review?(gsquelart) → review+
Comment on attachment 8766119 [details]
Bug 1267918 - Add GMPCrashHelperHolder to automatically disconnect crash helpers on actor destory.

https://reviewboard.mozilla.org/r/61146/#review58072

::: dom/media/gmp/GMPCrashHelperHolder.h:45
(Diff revision 1)
> +// So we need to ensure that in the content process, the GMPCrashHelpers stay
> +// connected to the GMPService until after ActorDestroy messages are received
> +// if there's an abnormal shutdown. In the case where the GMP doesn't crash,
> +// we do actually want to disconnect GMPCrashHandlers in ActorDestroy, since
> +// we don't have any other signal that a GMP actor is shutting down. If we don't
> +// disconnect the crash helper there in the abnormal shutdown case, the helper

'abnormal' -> 'normal'?
(If you did mean 'abnormal', then I don't quite understand what this sentence does in this paragraph, since you talk about the crashing case in the following paragraph.)

::: dom/media/gmp/GMPCrashHelperHolder.h:53
(Diff revision 1)
> +// In the crashing case, the GMPCrashHelpers are deallocated when the crash
> +// report is processed in GeckoMediaPluginService::RunPluginCrashCallbacks().
> +//
> +// It's a bit yuck that we have to have two paths for disconnecting the crash
> +// helpers, but there aren't really any better options.
> +class GMPCrashHelperHolder {

Style: '{' on separate line.

::: dom/media/gmp/GMPCrashHelperHolder.h:55
(Diff revision 1)
> +//
> +// It's a bit yuck that we have to have two paths for disconnecting the crash
> +// helpers, but there aren't really any better options.
> +class GMPCrashHelperHolder {
> +public:
> +  void SetCrashHelper(GMPCrashHelper* aHelper) {

Style: '{' on separate line.
Same with the other methods below.
Attachment #8766120 - Flags: review?(gsquelart) → review+
Comment on attachment 8766120 [details]
Bug 1267918 - Add GMPCrashHelper for MediaKeys.

https://reviewboard.mozilla.org/r/61148/#review58080

::: dom/media/eme/MediaKeys.h:40
(Diff revision 1)
>  typedef nsRefPtrHashtable<nsUint32HashKey, dom::DetailedPromise> PromiseHashMap;
>  typedef nsRefPtrHashtable<nsUint32HashKey, MediaKeySession> PendingKeySessionsHashMap;
>  typedef uint32_t PromiseId;
>  
>  // This class is used on the main thread only.
>  // Note: it's addref/release is not (and can't be) thread safe!

"it's" -> "Its"

::: dom/media/eme/MediaKeys.cpp:287
(Diff revision 1)
>      promise->MaybeResolve(JS::UndefinedHandleValue);
>    }
>    MOZ_ASSERT(!mPromises.Contains(aId));
>  }
>  
> +class MediaKeysGMPCrashHelper : public GMPCrashHelper {

Style: '{' on separate line.

::: dom/media/eme/MediaKeys.cpp:295
(Diff revision 1)
> +    : mMediaKeys(aMediaKeys)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread()); // WeakPtr isn't thread safe.
> +  }
> +  already_AddRefed<nsPIDOMWindowInner>
> +  GetPluginCrashedEventTarget() override {

Style: '{' on separate line.

::: dom/media/eme/MediaKeys.cpp:297
(Diff revision 1)
> +    MOZ_ASSERT(NS_IsMainThread()); // WeakPtr isn't thread safe.
> +  }
> +  already_AddRefed<nsPIDOMWindowInner>
> +  GetPluginCrashedEventTarget() override {
> +    MOZ_ASSERT(NS_IsMainThread()); // WeakPtr isn't thread safe.
> +    EME_LOG("MediaKeysGMPCrashHelper::GetPluginCrashedEventTargee()");

"Targee" -> "Target"
Attachment #8766121 - Flags: review?(gsquelart) → review+
Comment on attachment 8766121 [details]
Bug 1267918 - Add GMPCrashHelper for HTMLMediaElement.

https://reviewboard.mozilla.org/r/61150/#review58086

::: dom/media/AbstractMediaDecoder.h:94
(Diff revision 1)
>    virtual MediaDecoderOwner* GetOwner() = 0;
>  
>    // Set by Reader if the current audio track can be offloaded
>    virtual void SetPlatformCanOffloadAudio(bool aCanOffloadAudio) {}
>  
> +  virtual GMPCrashHelper* GetCrashHelper() { return nullptr; }

If possible, wouldn't it be safer to return an already_AddRefed<GMPCrashHelper>?

::: dom/media/MediaDecoder.cpp:1112
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    return mShuttingDown || mOwner->HasError();
>  }
>  
> +class MediaElementGMPCrashHelper : public GMPCrashHelper {

Style: '{' on separate line.

::: dom/media/MediaDecoder.cpp:1119
(Diff revision 1)
> +  explicit MediaElementGMPCrashHelper(HTMLMediaElement* aElement)
> +    : mElement(aElement)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread()); // WeakPtr isn't thread safe.
> +  }
> +  already_AddRefed<nsPIDOMWindowInner> GetPluginCrashedEventTarget() override {

Style: '{' on separate line.
Comment on attachment 8766123 [details]
Bug 1267918 - Remove obsolete GMP crash handling code.

https://reviewboard.mozilla.org/r/61154/#review58092

Sniff, my first media code at Mozilla...
I suppose the world is probably a better place without it. :-P
Attachment #8766123 - Flags: review?(gsquelart) → review+
https://reviewboard.mozilla.org/r/61152/#review58034

> I guess this is still true.
> 
> That means mCrashHelper's nsWeakPtr could be destroyed on a non-main thread.
> 
> I expect that would be bad, if it is non-null, because nsWeakReference doesn't have thread-safe ref-counting.

I will make the Release() implementation ensure that the instance is destroyed on the main thread.

> I wonder whether creating single-use BufferDecoderGMPCrashHelper here may be an option, to save an allocation for what is usually
> unused.
> 
> That's not worth losing sleep over.
> 
> But perhaps moving the nsWeakPtr to the BufferDecoder might make it easier to clear the nsWeakPtr at an appropriate time.

We'll call this function for every MIME type except Ogg and Wave, even if the decoder isn't backed by a GMP. Because we can't tell in advance whether our decoding will be done by GMP.

So I think it's not easy to avoid this allocation.

> MOZ_ASSERT(NS_IsMainThread()) please.
> 
> Similarly in the destructor.

I'll assert in the GMPCrashHelper destructor.
This means we can return already_AddRefed<T> for any RefPtr<T>s
being held as instance variables easier.

Review commit: https://reviewboard.mozilla.org/r/61472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61472/
Attachment #8766637 - Flags: review?(nfroyd)
Attachment #8766638 - Flags: review?(karlt)
Attachment #8766122 - Flags: review- → review?(karlt)
Comment on attachment 8766117 [details]
Bug 1267918 - Split LoadGMP message into select and load messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61142/diff/1-2/
Comment on attachment 8766118 [details]
Bug 1267918 - Add GMPCrashHelper to GMPService::GetGMP* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61144/diff/1-2/
Comment on attachment 8766119 [details]
Bug 1267918 - Add GMPCrashHelperHolder to automatically disconnect crash helpers on actor destory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61146/diff/1-2/
Comment on attachment 8766120 [details]
Bug 1267918 - Add GMPCrashHelper for MediaKeys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61148/diff/1-2/
Comment on attachment 8766121 [details]
Bug 1267918 - Add GMPCrashHelper for HTMLMediaElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61150/diff/1-2/
Comment on attachment 8766122 [details]
Bug 1267918 - Add GMPCrashHelper for WebAudio buffer decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61152/diff/1-2/
Comment on attachment 8766123 [details]
Bug 1267918 - Remove obsolete GMP crash handling code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61154/diff/1-2/
Comment on attachment 8766637 [details]
Bug 1267918 - Add do_AddRef(const RefPtr<T>& aObj).

https://reviewboard.mozilla.org/r/61472/#review58378

I have no objection in principle, but why does the T* overload not work for this case, given RefPtr's implicit conversion to T*?

r=me assuming there's a reasonable answer to that question.
Attachment #8766637 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #40)
> I have no objection in principle, but why does the T* overload not work for
> this case, given RefPtr's implicit conversion to T*?
> 
> r=me assuming there's a reasonable answer to that question.

I see that the compiler complains.  *shrug* C++.
Comment on attachment 8766638 [details]
Bug 1267918 - Ensure GMPCrashHelper instances are destroyed on the main thread.

https://reviewboard.mozilla.org/r/61474/#review58360

::: dom/media/gmp/GMPService.h:36
(Diff revision 1)
> -  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(GMPCrashHelper)
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef(void);
> +  NS_IMETHOD_(MozExternalRefCountType) Release(void);

I don't think these need to be virtual.

NS_METHOD_ would be consistent with NS_INLINE_DECL_THREADSAFE_REFCOUNTING, but
even that may be unnecessary.

::: dom/media/gmp/GMPService.h:49
(Diff revision 1)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +  void Destroy();
> +  mozilla::ThreadSafeAutoRefCnt mRefCnt;
> +  NS_DECL_OWNINGTHREAD

Odd.  NS_INLINE_DECL_THREADSAFE_REFCOUNTING didn't have this but compiling NS_IMPL_ADDREF needs this to compile, even though it is not used because mRefCnt.isThreadSafe.

::: dom/media/gmp/GMPService.cpp:672
(Diff revision 1)
> +    if (NS_FAILED(rv)) {
> +      delete this;
> +    }

Better to leak here than risk mishandling memory.  That's what NS_ReleaseOnMainThread does.
NS_DispatchToMainThread() will issue a warning, so no need for another, but MOZ_ALWAYS_SUCCEEDS() is available if you like.
Attachment #8766638 - Flags: review?(karlt) → review+
Attachment #8766122 - Flags: review?(karlt) → review+
https://reviewboard.mozilla.org/r/61474/#review58360

> I don't think these need to be virtual.
> 
> NS_METHOD_ would be consistent with NS_INLINE_DECL_THREADSAFE_REFCOUNTING, but
> even that may be unnecessary.

They don't need to be in themselves, but the NS_IMPL_ADDREF() and NS_IMPL_RELEASE_WITH_DESTROY() macros on Windows assume stdcall convention.

So they'll need to be "MozExternalRefCountType NS_STDCALL AddRef(void);" etc.
Comment on attachment 8766637 [details]
Bug 1267918 - Add do_AddRef(const RefPtr<T>& aObj).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61472/diff/1-2/
Comment on attachment 8766117 [details]
Bug 1267918 - Split LoadGMP message into select and load messages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61142/diff/2-3/
Comment on attachment 8766118 [details]
Bug 1267918 - Add GMPCrashHelper to GMPService::GetGMP* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61144/diff/2-3/
Comment on attachment 8766638 [details]
Bug 1267918 - Ensure GMPCrashHelper instances are destroyed on the main thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61474/diff/1-2/
Comment on attachment 8766119 [details]
Bug 1267918 - Add GMPCrashHelperHolder to automatically disconnect crash helpers on actor destory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61146/diff/2-3/
Comment on attachment 8766120 [details]
Bug 1267918 - Add GMPCrashHelper for MediaKeys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61148/diff/2-3/
Comment on attachment 8766121 [details]
Bug 1267918 - Add GMPCrashHelper for HTMLMediaElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61150/diff/2-3/
Comment on attachment 8766122 [details]
Bug 1267918 - Add GMPCrashHelper for WebAudio buffer decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61152/diff/2-3/
Comment on attachment 8766123 [details]
Bug 1267918 - Remove obsolete GMP crash handling code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61154/diff/2-3/
Comment on attachment 8766638 [details]
Bug 1267918 - Ensure GMPCrashHelper instances are destroyed on the main thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61474/diff/2-3/
Comment on attachment 8766119 [details]
Bug 1267918 - Add GMPCrashHelperHolder to automatically disconnect crash helpers on actor destory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61146/diff/3-4/
Comment on attachment 8766120 [details]
Bug 1267918 - Add GMPCrashHelper for MediaKeys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61148/diff/3-4/
Comment on attachment 8766121 [details]
Bug 1267918 - Add GMPCrashHelper for HTMLMediaElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61150/diff/3-4/
Comment on attachment 8766122 [details]
Bug 1267918 - Add GMPCrashHelper for WebAudio buffer decoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61152/diff/3-4/
Comment on attachment 8766123 [details]
Bug 1267918 - Remove obsolete GMP crash handling code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61154/diff/3-4/
https://hg.mozilla.org/integration/mozilla-inbound/rev/45fb3e6f39369db6abc5479bf2e129163352de88
Bug 1267918 - Add do_AddRef(const RefPtr<T>& aObj). r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/d409232655857c1d27209a02be589344f5eee903
Bug 1267918 - Split LoadGMP message into select and load messages. r=gerald

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdad12ea4de6852849f32dec2c4e934e911067c
Bug 1267918 - Add GMPCrashHelper to GMPService::GetGMP* functions. r=gerald

https://hg.mozilla.org/integration/mozilla-inbound/rev/94feba0c91327fb20850c5e7224d81b0ac72af2b
Bug 1267918 - Ensure GMPCrashHelper instances are destroyed on the main thread. r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/d46af3143920e130e065b924b73fe301d44328a0
Bug 1267918 - Add GMPCrashHelperHolder to automatically disconnect crash helpers on actor destory. r=gerald

https://hg.mozilla.org/integration/mozilla-inbound/rev/af36ac0decf7f830e82765eeb02daa04f9270e33
Bug 1267918 - Add GMPCrashHelper for MediaKeys. r=gerald

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ad41829d68ff2a46d21b65bb6a23dab97fb61c
Bug 1267918 - Add GMPCrashHelper for HTMLMediaElement. r=gerald

https://hg.mozilla.org/integration/mozilla-inbound/rev/86d9f7174e213b464f179eb5effd5c61a7ffa85d
Bug 1267918 - Add GMPCrashHelper for WebAudio buffer decoder. r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c72118d085a5cd547f83371435aad8fc7a96335
Bug 1267918 - Remove obsolete GMP crash handling code. r=gerald
You need to log in before you can comment on or make changes to this bug.