Closed Bug 796873 Opened 10 years ago Closed 10 years ago

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


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

Not set





(Reporter: Ms2ger, Assigned: AMoz)



(1 file, 3 obsolete files)

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#
nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(GetOwner())
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 ?
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:

For this bug, you can search for the string |GetLockOrientationPermission()| from the code snippet provided above by doing this:^[^\0]*%24&hitlimit=&tree=mozilla-central
Attached patch first patch ! (obsolete) — Splinter Review
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 !
Attachment #687843 - Flags: feedback?
Attachment #687843 - Flags: feedback? → feedback?(Ms2ger)
Comment on attachment 687843 [details] [diff] [review]
first patch !

Mounir, I know nothing about this code.
Attachment #687843 - Flags: feedback?(Ms2ger) → feedback?(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 ;)
Attachment #687843 - Flags: feedback?(mounir) → feedback-
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 !
Attached patch test patch (obsolete) — Splinter Review
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.
Attachment #687843 - Attachment is obsolete: true
Attachment #689722 - Flags: feedback?
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 ':' ;)
Attachment #689722 - Flags: feedback?
Attached patch bug-796873-fix (obsolete) — Splinter Review
Attachment #689722 - Attachment is obsolete: true
Attachment #689726 - Flags: feedback?(mounir)
Comment on attachment 689726 [details] [diff] [review]

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

Could you update the comments?

Attachment #689726 - Flags: feedback?(mounir) → feedback+
is this correct. Since the first comment was after the < LockScreenOrientation(orientation) > check, i have shifted it later.
Attachment #689737 - Flags: feedback?(mounir)
i dont understand why the patch is looking so weird ?
Attachment #689737 - Flags: feedback?(mounir) → review+
Assignee: nobody → amod.narvekar
Attachment #689726 - Attachment is obsolete: true
Target Milestone: --- → mozilla20
any more file is needed to modify ?
i searched at^[^\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 !
No, nothing else is needed. I pushed your patch to mozilla-inbound. It should be in mozilla-central soon.

Thank you for your contribution :)
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.