Closed Bug 1340189 Opened 7 years ago Closed 7 years ago

[GMP] Invalid removal in array during a loop

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: jya, Assigned: jya)

Details

Attachments

(3 obsolete files) 

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/media/gmp/GMPCDMProxy.cpp#728

the loop:
  for (size_t i = 0; i < mDecryptionJobs.Length(); i++) {
    DecryptJob* job = mDecryptionJobs[i];
    if (job->mId == aId) {
#ifdef DEBUG
      jobIdFound = true;
#endif
      job->PostResult(aResult, aDecryptedData);
      mDecryptionJobs.RemoveElementAt(i);
    }

the element shouldn't be removed from the list while parsing it. entries will be missed otherwise.
See Also: → 1340180
See Also: 1340180 → 
Comment on attachment 8838299 [details]
Bug 1340189: P1. Only clear array once parsing is done.

https://reviewboard.mozilla.org/r/113256/#review114706

Drive-by r-, because the array should not be completely cleared! Only some items with a particular id are to be removed.
I think the easiest solution here would be a second loop (I'm assuming the posting order is important) going backwards and removing only the appropriate elements.
Attachment #8838299 - Flags: review-
Comment on attachment 8838300 [details]
Bug 1340189: P2. Fix style and on the fly changes.

https://reviewboard.mozilla.org/r/113258/#review114712

Some nits below. Frankly I stopped caring about style after a quarter of this billion-line patch. -- Still went through all of it, didn't notice any logic-breaking changes.

Forwarding r? to cpearce, as he's the main maintainer of dom/media/gmp, so he should decide whether this mass restyling is welcome in his domain, especially since we're now not 100% clear about some of them.

::: dom/media/gmp/GMPChild.cpp:110
(Diff revision 1)
> +    NS_LITERAL_STRING("lib") + baseName + NS_LITERAL_STRING(".so");
>  #elif defined(XP_WIN)
> -  nsAutoString binaryName =                            baseName + NS_LITERAL_STRING(".dll");
> +  nsAutoString binaryName = baseName + NS_LITERAL_STRING(".dll");

This last line should be aligned with NS_LITERAL... above, i.e., 2 spaces to the right.

::: dom/media/gmp/GMPChild.cpp:241
(Diff revision 1)
> -  LOGD("%s pluginPath=%s", __FUNCTION__, NS_ConvertUTF16toUTF8(aPluginPath).get());
> +  LOGD(
> +    "%s pluginPath=%s", __FUNCTION__, NS_ConvertUTF16toUTF8(aPluginPath).get());

[argumentative]
I think there's something I just don't like about clang-format's idea of splitting function declarations&calls!
In particular splitting between '(' and the first parameter/argument, I think it should be the last resort.

In this case, I'd have preferred:
```
  LOGD("%s pluginPath=%s",
       __FUNCTION__,
       NS_ConvertUTF16toUTF8(aPluginPath).get());
```
I.e.: Split *between* arguments first, and align arguments with each other.
Yes it takes one more line, but I think it's clearer.

Just an opinion, feel free to proceed, we can always reformat everything again later on, if needed. ;-)

::: dom/media/gmp/GMPChild.cpp:387
(Diff revision 1)
> -    MOZ_ASSERT_IF(aWhy == NormalShutdown, !mGMPContentChildren[i - 1]->IsUsed());
> +    MOZ_ASSERT_IF(aWhy == NormalShutdown,
> +                  !mGMPContentChildren[i - 1]->IsUsed());

See, splitting between args here, perfect!

::: dom/media/gmp/GMPCrashHelper.h:27
(Diff revision 1)
> -  virtual already_AddRefed<nsPIDOMWindowInner> GetPluginCrashedEventTarget() = 0;
> +  virtual already_AddRefed<nsPIDOMWindowInner>
> +  GetPluginCrashedEventTarget() = 0;

See, splitting after the return type, perfect! :-P

::: dom/media/gmp/GMPDecryptorChild.cpp:61
(Diff revision 1)
> -    RefPtr<mozilla::Runnable> t =
> -      dont_add_new_uses_of_this::NewRunnableMethod(this, m, aMethod, Forward<ParamType>(aParams)...);
> +    RefPtr<mozilla::Runnable> t = dont_add_new_uses_of_this::NewRunnableMethod(
> +      this, m, aMethod, Forward<ParamType>(aParams)...);

In this case, I would argue that because '=' has lower precedence than '(' (function call), this line should first be split at the '='.
And then because the function call still doesn't fit, it should be split between arguments.

Is clang-format's priority to have the fewest lines possible?
Comment on attachment 8838301 [details]
Bug 1340189: P3. Fix coding style.

https://reviewboard.mozilla.org/r/113260/#review114730

A couple of nits I've picked up...

Forwarding r? to cpearce for final approval, as he's the benevolent dictator of dom/media/evilme.

::: dom/media/eme/MediaKeySystemAccess.cpp:344
(Diff revision 1)
> -      typedef struct {
> +      typedef struct
> +      {
>          const nsCString& mMimeType;
>          const nsCString& mEMECodecType;
>          const char16_t* mCodecType;
>          KeySystemContainerSupport* mSupportType;
>        } DataForValidation;

'typedef struct {...} DataForValidation;' -> 'struct DataForValidation {...};'.
(Maybe need a new rule for that?)

::: dom/media/eme/MediaKeySystemAccess.cpp:478
(Diff revision 1)
> -    strings[static_cast<uint32_t>(MediaKeySessionType::Persistent_license)].value;
> +    strings[static_cast<uint32_t>(MediaKeySessionType::Persistent_license)]
> +      .value;

Should `.value` really be indented?
(Seen others like this below.)
Comment on attachment 8838300 [details]
Bug 1340189: P2. Fix style and on the fly changes.

https://reviewboard.mozilla.org/r/113258/#review114738

I am refactoring this code, and rather tired of spending a quarter of my time having to resolve merge conflicts. So I'd prefer to hold off on this.

Also, I'd prefer to format the code I spend most of my time working in myself. I'll look at the formating of dom/media/gmp after I'm done with Bug 1315850. That will reduce unnecessary churn.
Attachment #8838300 - Flags: review?(cpearce) → review-
Comment on attachment 8838301 [details]
Bug 1340189: P3. Fix coding style.

https://reviewboard.mozilla.org/r/113260/#review114740

I am refactoring this code, and rather tired of spending a quarter of my time having to resolve merge conflicts. So I'd prefer to hold off on this.

Also, I'd prefer to format the code I spend most of my time working in myself. I'll look at the formating of dom/media/gmp after I'm done with Bug 1315850. That will reduce unnecessary churn.
Attachment #8838301 - Flags: review?(cpearce) → review-
Comment on attachment 8838299 [details]
Bug 1340189: P1. Only clear array once parsing is done.

https://reviewboard.mozilla.org/r/113256/#review114742

::: dom/media/gmp/GMPCDMProxy.cpp:734
(Diff revision 1)
>        jobIdFound = true;
>  #endif
>        job->PostResult(aResult, aDecryptedData);
> -      mDecryptionJobs.RemoveElementAt(i);
>      }
> +    mDecryptionJobs.Clear();

There are a couple of things here:
* Since your change to ensure that we only have one decode operation in progress at once, the array here should only have at most one entry.
* The ids here should be unique. So in the previous case where we had multiple decodes/decrypts enqueued at once, this loop would only ever have removed a single element from the array anyway. I should have put a "break" statement after the removal. That was dumb of me.
* If you clear mDecryptionJobs here, as in this patch, if the result wasn't the first result, you'd blow away everything in the array! But since there should be only 1 thing in the array since you now guarantee that we'll only have one decode in progress at once, that shouldn't happen. So you're succeeding with this patch by accident.

In my work in Bug 1315850, I changed this code to assume a single decrypt in progress at once. In the ChromiumCDMProxy at least. I'll be deleting this code once I've landed and stabilized my work in  Bug 1315850.

Please just add a "break" after the RemoveElementAt().
Attachment #8838299 - Flags: review?(cpearce) → review-
Comment on attachment 8838300 [details]
Bug 1340189: P2. Fix style and on the fly changes.

(to match cpearce's r-)
Attachment #8838300 - Flags: review?(gsquelart) → review-
Comment on attachment 8838301 [details]
Bug 1340189: P3. Fix coding style.

(to match cpearce's r-)
Attachment #8838301 - Flags: review?(gsquelart) → review-
Attachment #8838299 - Attachment is obsolete: true
Attachment #8838300 - Attachment is obsolete: true
Attachment #8838301 - Attachment is obsolete: true
The code, and logic is still wrong in this code.

The changes I made also had the other benefits of making local code static, cleaning up incorrect C++ definition (wrong explicit keyword), simplifying loop with C++11 iterators and so on.

So up to you to use it or not.
This is in the old CDM backend. No longer needed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: