Closed Bug 1241991 Opened 8 years ago Closed 8 years ago

Convert APZCTreeManager::mTreeLock to a Mutex

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: kats, Assigned: simplysourabh_123, Mentored)

Details

(Whiteboard: [good first bug][lang=c++][gfx-noted])

Attachments

(1 file, 2 obsolete files)

bugfix-1241991-revised.diff
12.80 KB, patch
kats
: review+
Details | Diff | Splinter Review 
Right now mTreeLock is a Monitor, but we don't really need the Monitor-ness of it, we can make do with just a Mutex.
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][gfx-noted]
Hello Kartikaya
I'd like to work on this bug. I'm new to Mozilla contribution. Can you guide me how to begin?
Hi Sourabh!

The first thing to do is to check out the source code and get it compiling. There are instructions to do this at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build - give that a shot and let me know how it goes. Once you are able to build the code successfully, I'll assign this bug to you and can provide some more details on what to do.
@Kartikaya: I've cloned the mozilla-central repository, and completed the build process as mentioned in the link.
Great! So in the gfx/layers/apz/src/APZCTreeManager.h file, there is a mTreeLock monitor field defined in the APZCTreeManager class [1]. What we want to do is replace that Monitor with a Mutex instead. There are some places that use the Monitor by creating a MonitorAutoLock [2], which is a RAII class to hold the Monitor. We will have to update those places to use a MutexAutoLock [3] instead. You should also search for other references to mTreeLock (such as in code comments) and see if any of them need updating similarly.

Once you have done that, the simplest way to test the code is to build it and run the gtests, which is one of the many test suites we have. In this case running that test suite should exercise the code enough to make sure that there are no problems. You can run the gtests using |mach gtest|. It will run a bunch of stuff including tests for the APZ code.

[1] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/gfx/layers/apz/src/APZCTreeManager.h#528
[2] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/xpcom/glue/Monitor.h#72
[3] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/xpcom/glue/Mutex.h#182
Assignee: nobody → simplysourabh_123
And once you have that done, you can submit the patch using MozReview, our code reviewing tool. For instructions on how to do that, take a look at http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html - the basic steps are (1) commit the patch to your local repository with an appropriate commit message, something like "Bug 1241991 - Switch mTreeLock from a Monitor to a Mutex. r?kats" and then (2) push the patch to our reviewboard server.
Ok, so I've changed the 'mTreeLock' field, from Monitor to Mutex and included "mozilla/Mutex.h" header in APZCTreeManager.h file. In APZCTreeManager.cpp I've changed 'lock' field from MonitorAutoLock to MutexAutoLock which uses mTreeLock as a parameter.

The build process and gtests were successful. Is there any other thing I should do, or any other dependency I should be aware of, related to mTreeLock, before submitting the patch?
Not that I can think of. Please go ahead and submit the patch for review. Thanks!
I am facing a little difficulty here. Firstly I've commited to my local repo with 'hg commit' command. But when I push it with 'hg push review' it says :

pushing to ssh://reviewboard-hg.mozilla.org/gecko
remote: Permission denied (publickey).
abort: no suitable response from remote hg!

Can you help?
Hm, the most likely problem is that your bugzilla API key isn't set up properly. Looking at the docs I linked to, it doesn't seem to actually talk about how to set that up, sorry! What you need to do is go to your Bugzilla preferences, and add a new API key under the API keys section. Then, in your ~/.hgrc file, add the following settings:

[bugzilla]
username = <your bugzilla login email>
apikey = <the api key generated>

I think that should give you the required permissions to push to the reviewboard server. I'll see if I can get the documentation page updated as well.
Actually, the API key setup is described on the page at http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-mercurial.html#mozreview-install-mercurial - if that doesn't work for you let me know, and we can try to figure out what the problem is.
I ran './mach mercurial-setup', and tried to push it again but it gave the same permission denied message. This page :
http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install.html

says that - if you are using SSH to push to MozReview, it requires an LDAP account.
Instructions for obtaining a Mozilla LDAP account with Mercurial access are documented at:
https://www.mozilla.org/en-US/about/governance/policies/commit/

Also there's a bug: Permission denied (publickey) when pushing to try with similar issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1078215
(In reply to Sourabh Shrivastava from comment #11)
> I ran './mach mercurial-setup', and tried to push it again but it gave the
> same permission denied message. This page :
> http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/
> install.html
> 
> says that - if you are using SSH to push to MozReview, it requires an LDAP
> account.

That is correct. To push with a Bugzilla account, you need to use HTTPS rather than SSH.

That means in your .hgrc file the "review" entry in the "paths" section should be an https:// URL - see the snippet at [1] for an example.

[1] http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-mercurial.html#mozreview-install-autoreview
(In reply to Botond Ballo [:botond] from comment #12)
> (In reply to Sourabh Shrivastava from comment #11)
> > I ran './mach mercurial-setup', and tried to push it again but it gave the
> > same permission denied message. This page :
> > http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/
> > install.html
> > 
> > says that - if you are using SSH to push to MozReview, it requires an LDAP
> > account.
> 
> That is correct. To push with a Bugzilla account, you need to use HTTPS
> rather than SSH.
> 
> That means in your .hgrc file the "review" entry in the "paths" section
> should be an https:// URL - see the snippet at [1] for an example.
> 
> [1]
> http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/
> install-mercurial.html#mozreview-install-autoreview

Thanks Bootond that worked.. But I'm facing another problem. When I perform 'hg push review' it says :

redirecting push to https://reviewboard-hg.mozilla.org/gecko
abort: cannot review empty changeset c7c11115c029
(add files to or remove changeset)

I've made changes to two files and then I performed commit before pushing. I can't understand how the changeset is empty. Can you help me with this?
(In reply to Sourabh Shrivastava from comment #13)
> Thanks Bootond that worked.. But I'm facing another problem. When I perform
> 'hg push review' it says :
> 
> redirecting push to https://reviewboard-hg.mozilla.org/gecko
> abort: cannot review empty changeset c7c11115c029
> (add files to or remove changeset)
> 
> I've made changes to two files and then I performed commit before pushing. I
> can't understand how the changeset is empty. Can you help me with this?

Could you do 'hg log', and paste the first few entries (up to the first one that's not yours)?
(Also note that it might be easier to debug this if you join us on IRC [1] - #introduction is a good channel to hang out in, or you can find botond or myself in #apz)

[1] https://wiki.mozilla.org/IRC
Hey, sorry I missed you on IRC. I should have noted that I'm usually on during the day eastern time, and I'm guessing you're in an incompatible timezone :(

Still, I saw your conversation with Gijs in #developers and I think what's happening is that when you're pushing, it's trying to push the wrong changeset. From the hg wip output it looked like 28786d7eb525 is the changeset you really want to be pushing, but when you run |hg push| it's trying to push c7c11115c029. So try doing |hg push -r 28786d7eb525 review| and that might work.

If that fails, do |hg export -r 28786d7eb525 > mypatch.txt| which should spit out a patch into the "mypatch.txt" file, and you can just attach it to this bug as an attachment.
Attached patch bug-1241991-fix (obsolete) — — Splinter Review 
1241991 Bug fix : mTreeLock changed from Monitor to Mutex
Comment on attachment 8714822 [details] [diff] [review]
bug-1241991-fix

Great, thanks! I'm flagging myself for review on this, I'll take a closer look in a bit.
Attachment #8714822 - Flags: review?(bugmail.mozilla)
Oh, so in bug 1243547 which landed a few days ago I added a new function APZCTreeManager which also uses mTreeLock - you'll need to update your codebase and the patch to account for this. Under normal circumstances, doing |hg pull --rebase| should do this but I'm not entirely sure of the state of your tree so I can't say that with certainty. In the worst case you can reapply the patch using the attachment on this bug.

Anyway, after doing the pull/rebase, do another build - you should see a compile error because of the new function I added (APZCTreeManager::AdjustScrollForSurfaceShift). Please update the patch accordingly, and upload a new version to this bug. Other than this, the patch looks fine. Nice work!

Sorry for the extra step, but it's good to learn how to do this as it tends to happen relatively frequently when working in Mozilla code.
Comment on attachment 8714822 [details] [diff] [review]
bug-1241991-fix

Clearing review flag for now. I see no issues with it though other than what I mentioned above.
Attachment #8714822 - Flags: review?(bugmail.mozilla)
Attached patch bug-1241991-fix-revised (obsolete) — — Splinter Review 
revised bug fix for 1241991: Modified MonitorAutoLock to MutexAutoLock in APZCTreeManager::AdjustScrollForSurfaceShift
Hey, sorry I wasn't more clear - I meant that you need to combine the two changes into one patch. You can do this using hg histedit command (see https://www.mercurial-scm.org/wiki/HisteditExtension) to fold the two patches together. Once you have done that please upload the combined patch. Also, for future reference, when uploading the new attachment, you can mark the old attachments obsolete (there are checkboxes on the attachment upload page for this) so that your new patch "replaces" the old patches on the bug. And you can flag me for review on the patch on the attachment upload page by changing the "review" flag to "?" and typing ":kats" in the text field that appears next to it. Let me know if you have any questions or if this doesn't make sense. Thanks!
I accidently collapsed two revisions and now I can't create a new patch ..
https://pastebin.mozilla.org/8858410
please help..
Ok, so from the pastebin you linked, I think you need to run these commands:

hg update 1cb9fb5fe80a  # this will check out 1cb9fb5fe80a
hg strip c3ba1d480356 # this will discard c3ba1d480356
hg strip 0aea4cd7ac79 # this will discard 0aea4cd7ac79
hg qpop # this should pop 1cb9fb5fe80a off the mq stack, and leave you at 73f6a358b013
hg qfold bug-1241991-fix-revised # this will squash your two mq patches together
hg qref -e # edit the commit message, which will probably be wrong now
hg export -r tip > mypatch.txt # export the patch
Combined patch for bugfix.
Attachment #8714822 - Attachment is obsolete: true
Attachment #8714931 - Attachment is obsolete: true
Attachment #8715166 - Flags: review?(bugmail.mozilla)
Comment on attachment 8715166 [details] [diff] [review]
bugfix-1241991-revised.diff

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

Great, thanks! The only thing off is the commit message, but I'll just copy the commit message from your previous patch when landing this.
Attachment #8715166 - Flags: review?(bugmail.mozilla) → review+
Completed my first bug fix.. Yayy !! Thanks Kartikaya for guiding me and being so much supportive throughout. It is a privilege for me, to work with you guys. Looking forward to contribute further to this awesome community.. :)
I received an email for creating mozilla account. Can you vouch me for this
https://mozillians.org/en-US/u/sshri/
Thanks!!
Thanks Sourabh! I submitted a vouch for you. If you're interested in working on further bugs, bug 1136323 might be a good one for you. It's fairly straightforward as well and would help you get more used to the mechanics of making changes before diving in to something more complex.
Thanks Kartikaya.. I'll look into that.
https://hg.mozilla.org/mozilla-central/rev/c04826d65dd3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: