Closed Bug 1316010 Opened 8 years ago Closed 3 years ago

[findbugs] [UL] org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

Categories

(Firefox for Android Graveyard :: General, defect, P3)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: sebastian, Assigned: svezauzeto12, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=java])

Attachments

(1 file, 1 obsolete file)

Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths
58 bytes, text/x-review-board-request
sebastian
: review+
gcp
: review+
Details 
org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

This method acquires a JSR-166 (java.util.concurrent) lock, but does not release it on all paths out of the method. In general, the correct idiom for using a JSR-166 lock is:

>    Lock l = ...;
>    l.lock();
>    try {
>        // do something
>    } finally {
>        l.unlock();
>    }
To start, set up a build environment - you can see the instructions here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Then, you'll need to upload a patch - see:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "sebastian" and you can find me and other helpful folks in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC
can I work on this ?

Thanks
(In reply to svezauzeto12 from comment #2)
> can I work on this ?

Yeah, do you need any additional help?
Not in this phase , but will probably need at some point. I will start working now.

Thanks
Okay I was wrong, I need help. I cannot find  org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10)

Did maybe folder for this changed since the bug was reported ?
I ran the check again and the issue is reported here:

> UL
> org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths
> Bug type UL_UNRELEASED_LOCK
> In class org.webrtc.videoengine.ViEAndroidGLES20
> In method org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10)
> At ViEAndroidGLES20.java:[line 319]
I see following function that you mentioned in #0 comment:

 public void onDrawFrame(GL10 gl) {
        nativeFunctionLock.lock();
        if(!nativeFunctionsRegisted || !surfaceCreated) {
            nativeFunctionLock.unlock();
            return;
        }

So , if !nativeFunctionsRegisted or !surfaceCreated the nativeFucntionLock will unlock. But If I do not understand what is the point of function if we do this (if this is what you wanted me to do ):

  public void onDrawFrame(GL10 gl) {
        nativeFunctionLock.lock();
        try{
        if(!nativeFunctionsRegisted || !surfaceCreated) {
            nativeFunctionLock.unlock();
            return;
        }
        finally(){
                nativeFunctionLock.unlock();
            }
        }


So the lock will unlock anyway, so why do we need if condition in the first place, if we will unlock it no matter is condition valid or not.

Thanks for explaining.
Maybe this simple solution will work ?
Assignee: nobody → svezauzeto12
Status: NEW → ASSIGNED
Please review upper commit when you have time so I know in which direction solution should go.

Thank you :)
Comment on attachment 8815470 [details]
Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/96368/#review97926

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViEAndroidGLES20.java:327
(Diff revision 1)
>              return;
>          }
>  
>          if(!openGLCreated) {
>              if(0 != CreateOpenGLNative(nativeObject, viewWidth, viewHeight)) {
> +                nativeFunctionLock.unlock();

This should work. However instead of trying to find all paths that need to call unlock, we should just wrap this in try {} finally {} like in comment 0.
Attachment #8815470 - Flags: review?(s.kaspari)
I have following problem

when I do

hg push -r 324892:9bff9330b828 review

I get error:

 
pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
searching for changes
no changes found
abort: cannot submit reviews referencing multiple bugs
(limit reviewed changesets with "-c" or "-r" arguments)

hg diff -c tip shows only changes made for this bug, nothing else.
Either push a single changeset:

> hg push -c changeset review

Or a range:

> hg push -r changeset:changeset review

All pushed commits need to reference the same bug in the commit message.
Comment on attachment 8815470 [details]
Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/96368/#review99854

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViEAndroidGLES20.java:319
(Diff revision 2)
>          }
>          return false;
>      }
>  
>      public void onDrawFrame(GL10 gl) {
> +        try{

nit: style: Space before {:
> try {

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViEAndroidGLES20.java:320
(Diff revision 2)
>          nativeFunctionLock.lock();
>          if(!nativeFunctionsRegisted || !surfaceCreated) {
> -            nativeFunctionLock.unlock();
>              return;
>          }
>  
>          if(!openGLCreated) {
>              if(0 != CreateOpenGLNative(nativeObject, viewWidth, viewHeight)) {
>                  return; // Failed to create OpenGL
>              }
>              openGLCreated = true; // Created OpenGL successfully
>          }
>          DrawNative(nativeObject); // Draw the new frame

this block should be indented (+4 spaces) now inside the try {} block.

::: media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViEAndroidGLES20.java:333
(Diff revision 2)
> +        }
> +        finally{

nit: style:
> } finally {
Attachment #8815470 - Flags: review?(s.kaspari)
Comment on attachment 8815470 [details]
Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/96368/#review99944

This patch now only contains the last changes, but not the previous ones. Did you accidentally create two commits and only pushed the last one?
Attachment #8815470 - Flags: review?(s.kaspari)
Can you please check it now ? 

I did hg up (last commit for this bug before today ) and the commited.

But when I can only oush with hg push -c , and not with -r ( I usually push with -r ), so maybe that causes some problems.
Please check only latest one, it should contain all changes.

Thank you
Please try to create a single commit from the two and push it to reviewboard. Otherwise I can't review and land just one of them.

If you are using Mercurial then you can do that with "hg histedit" (See 'fold' and do not forget to update the commit message):
https://www.mercurial-scm.org/wiki/HisteditExtension

And if you are using Git then an interactive rebase might be what you want:
https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History
Comment on attachment 8815470 [details]
Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/96368/#review100158
Attachment #8815470 - Flags: review?(s.kaspari) → review-
Comment on attachment 8820010 [details]
Bug 1316010 -  org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/99562/#review100160
Attachment #8820010 - Flags: review?(s.kaspari)
Since I made a lot of mess in my local hg , and my skills on hg are low I think it would be easier for me just to make new commit from beginning, it is pretty simple commit so I wouldn't lose much time. Would this work for you ?
Yeah, that's fine too.
I don't know what is happening, I have following problem. I made new folder, downloaded fresh again and made changes, then commited. 

this is hg diff -c tip output 

https://pastebin.mozilla.org/8953880

and this is hg log --graph

https://pastebin.mozilla.org/8953881

usually hg push -r xxx review works just fine, but this time I get error:

hg push -r dedd11d21ff6 review
pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
searching for changes
no changes found
abort: cannot submit reviews referencing multiple bugs
(limit reviewed changesets with "-c" or "-r" arguments)


Same thing happend on previous commits, but worked when pushed with -c instead of -r, but not sure will that mess up commit again.
(In reply to Tomislav Jurin from comment #28)
> and this is hg log --graph
> 
> https://pastebin.mozilla.org/8953881

What's weird here is that this patch is not on top of central but on top of a different changeset. If you look at "hg log -f" you will see just the changesets in this sub tree and then you might be able to see what state your patch is on top of. You could try rebasing but this depends on what other changes are in line (see below).

> usually hg push -r xxx review works just fine, but this time I get error:
> 
> hg push -r dedd11d21ff6 review
> pushing to https://reviewboard-hg.mozilla.org/autoreview
> searching for appropriate review repository
> redirecting push to https://reviewboard-hg.mozilla.org/gecko
> searching for changes
> no changes found
> abort: cannot submit reviews referencing multiple bugs
> (limit reviewed changesets with "-c" or "-r" arguments)

With "hg log -f" you will see what those other changes are.

> Same thing happend on previous commits, but worked when pushed with -c
> instead of -r, but not sure will that mess up commit again.

Just pushing the single changeset (-c) is okay as long as this single changeset is everything that is needed here. Previously your changes where distributed over multiple commits, so just pushing one of them doesn't include all changes.
Attachment #8820010 - Attachment is obsolete: true
Comment on attachment 8815470 [details]
Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/96368/#review102422

This looks good to me now. I'll flag gcp additionally - as this is touching webrtc code.
Attachment #8815470 - Flags: review?(s.kaspari) → review+
Attachment #8815470 - Flags: review?(gpascutto)
Comment on attachment 8815470 [details]
Bug 1316010 - fixing org.webrtc.videoengine.ViEAndroidGLES20.onDrawFrame(GL10) does not release lock on all paths

https://reviewboard.mozilla.org/r/96368/#review103766

If this source still exists in the upstream WebRTC sources it may be a good idea to push the fix upstream. (But they rewrote large parts again and I think this code might have been part of it)
Attachment #8815470 - Flags: review?(gpascutto) → review+
Keywords: good-first-bug
Whiteboard: [lang=java][good first bug] → [lang=java]
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: