796873 - Check the null GetOwner() case in nsScreen::MozLockOrientation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

See bug 793244 comment 5.

Comment 1

[Heilman_IV]

Possibly a non-issue.
Can the owner be removed in another thread between GetLock and GetOwner?
Can do_QueryInterface fail?
If both are no, then this is a non-issue and there is no need for the if statement.

switch (GetLockOrientationPermission()) {
#In GetLockOrientationPermission#
  if (!owner) {
    return LOCK_DENIED;
  }
#Back to switch#
...
case FULLSCREEN_LOCK_ALLOWED:
nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(GetOwner())

Comment 2

[Amod [:AMoz]]

i am interested in working in this. 
I have build src code long back.
Do i need to build anything else ?
Also which files do i need to edit ?

Comment 3

[Mark Capella [:capella]]

You can search for the affected files referred to in the bug by searching the mozilla source code directory, using as a search string some of the code posted above ...

To start a source code search of any kind start here: http://mxr.mozilla.org/mozilla-central/search

For this bug, you can search for the string |GetLockOrientationPermission()| from the code snippet provided above by doing this: http://mxr.mozilla.org/mozilla-central/search?string=GetLockOrientationPermission%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 4

[Amod [:AMoz]]

Attachment: first patch !

i have found the required | switch | scenario in only one file, so making changes in that file only ! If there is any other case then please suggest me.. Thanks !

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 687843 [details] [diff] [review]
first patch !

Mounir, I know nothing about this code.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 687843 [details] [diff] [review]
first patch !

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

You shouldn't remove the LockScreenOrientation() call, otherwise we will not lock the screen as we should. However, you could move that call *after* setting |target| and make target == null case returning false. That way, we will not try to lock the screen if we haven't a way to unlock it.

Thank you for working on that on sorry for being unresponsive on IRC, I am online 100% of the time but not actually in front of my computer ;)

Comment 7

[Amod [:AMoz]]

i will return to the bug after two days after my next paper of the exam schedule gets over. then i will upload the patch with the suggested changes. thanks !

Comment 8

[Amod [:AMoz]]

Attachment: test patch

is it as per you suggested ?

Also i didnot request feedback on your name since when i typed "mounir" in the text box, it gave a list of names, so i was confused.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 689722 [details] [diff] [review]
test patch

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

You should try to lock after checking if target is null.

Also, you should remove the XXX comment and, "( target == null )" should be "(!target)".

Could you attach another patch and ask for a review to ":mounir" (notice the ':' ;)

Comment 10

[Amod [:AMoz]]

Attachment: bug-796873-fix

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 689726 [details] [diff] [review]
bug-796873-fix

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

Could you update the comments?

Thanks

Comment 12

[Amod [:AMoz]]

Attachment: update comments !

is this correct. Since the first comment was after the < LockScreenOrientation(orientation) > check, i have shifted it later.

Comment 13

[Amod [:AMoz]]

i dont understand why the patch is looking so weird ?

Comment 14

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/439572ceb86b

Comment 15

[Amod [:AMoz]]

any more file is needed to modify ?
i searched at  http://mxr.mozilla.org/mozilla-central/search?string=GetLockOrientationPermission%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central where i found only i file where the switch case is used which i have modified and uploaded the patch !

Comment 16

[Mounir Lamouri (:mounir)]

No, nothing else is needed. I pushed your patch to mozilla-inbound. It should be in mozilla-central soon.

Thank you for your contribution :)

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/439572ceb86b