Replace runInContent() with ContentTask.spawn()

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: smacleod, Assigned: ajkerrigan, Mentored)

Tracking

Trunk
Firefox 46
Points:
2
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [good first bug])

User Story

Session Restore tests have a helper function runInContent()[1] which is used to execute functions in the Content Process (There is documentation on MDN which is helpful for developing multi-process firefox[2][3]).

Recently Bug 1107609 added a more general ContentTask.spawn()[4] method for executing functions in the content process. We should replace all of the uses of runInContent()[5] with the new method, and then remove the implementation of runInContent()[1] so that no new tests use it.

The two methods take slightly different arguments, so it will be a little more complex than just replacing the method name, but should still be pretty simple.

The affected tests can be executed from the mozilla-central checkout by running:
 $ ./mach mochitest-browser browser/components/sessionstore/test/

Or, the tests that currently use runInContent can be run individually (and more quickly) by running these commands:
 $ ./mach mochitest-browser browser/components/sessionstore/test/browser_447951.js
 $ ./mach mochitest-browser browser/components/sessionstore/test/browser_500328.js

A successful patch for this bug will have 
- All uses of runInContent replaced and adapted to the new ContentTask.spawn method
- The definition of runInContent removed
- All the sessionstore mochitest-browser tests passing.

[1] https://dxr.mozilla.org/mozilla-central/search?q=%22function+runInContent%22+path%3Abrowser%2Fcomponents%2Fsessionstore%2Ftest%2F*&redirect=true
[2] https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox
[3] https://developer.mozilla.org/en-US/Add-ons/Working_with_multiprocess_Firefox

[4] https://hg.mozilla.org/integration/fx-team/file/2483a2d08384/testing/mochitest/BrowserTestUtils/ContentTask.jsm#l52
[5] https://dxr.mozilla.org/mozilla-central/search?q=%22runInContent%22&redirect=true

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Now that Bug 1107609 has landed we should replace runInContent() uses with ContentTask.spawn() and then remove it from head.js
Flags: needinfo?(mhoye)
Flags: firefox-backlog+
(Reporter)

Updated

4 years ago
User Story: (updated)
(Reporter)

Updated

4 years ago
Flags: qe-verify-
Nice.
Flags: needinfo?(mhoye)
Whiteboard: [good first bug]

Comment 2

4 years ago
I have my environment set up and I am working on this bug. Is that alright?
(Reporter)

Comment 3

4 years ago
(In reply to cbazoian12 from comment #2)
> I have my environment set up and I am working on this bug. Is that alright?

Yes, that's great! I've assigned the bug to you.

Please let me know when you have any questions or would like help.
Assignee: nobody → cbazoian12
Status: NEW → ASSIGNED

Comment 4

4 years ago
Awesome. Thank you
(Reporter)

Comment 5

4 years ago
Hey cbazoian12,

I wanted to check in and see how you're progressing with this bug? Is there anything I can do to help you moving forward?
Flags: needinfo?(cbazoian12)

Comment 6

4 years ago
(In reply to Steven MacLeod [:smacleod] from comment #5)
> Hey cbazoian12,
> 
> I wanted to check in and see how you're progressing with this bug? Is there
> anything I can do to help you moving forward?

Just to make sure since I've never submitted a patch before, do I submit the diff file as the patch? And once I have the diff file, how do you want me to submit it?
Flags: needinfo?(cbazoian12)
(Reporter)

Comment 7

4 years ago
(In reply to cbazoian12 from comment #6)
> Just to make sure since I've never submitted a patch before, do I submit the
> diff file as the patch? And once I have the diff file, how do you want me to
> submit it?

Yes you'll submit the diff file as the patch. You submit it by uploading it to this bug. For more specifics about how to do this we have some great documentation[1]. Let me know if you have any more questions!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 8

4 years ago
Perfect. Thank you.

Comment 9

4 years ago
Posted patch Patch for bug 1132556 (obsolete) — Splinter Review
I am hoping I did this right. I attached the patch with the diff file in it.
Flags: needinfo?(smacleod)
(Reporter)

Comment 10

4 years ago
The patch that was attached doesn't appear to be your changes, it looks like a diff introducing a bunch of the files that already exist in the repository. (In the future you'll also want to mark review?smacleod@mozilla.com rather than needinfo?smacleod@mozilla.com).

What command are you using to generate this patch for upload?
Flags: needinfo?(smacleod) → needinfo?(cbazoian12)

Comment 11

4 years ago
(In reply to Steven MacLeod [:smacleod] from comment #10)
> The patch that was attached doesn't appear to be your changes, it looks like
> a diff introducing a bunch of the files that already exist in the
> repository. (In the future you'll also want to mark
> review?smacleod@mozilla.com rather than needinfo?smacleod@mozilla.com).
> 
> What command are you using to generate this patch for upload?

I was using the link http://jungels.net/articles/diff-patch-ten-minutes.html. But I believe I followed the instructions wrong. I'll go back and try again.
Flags: needinfo?(cbazoian12)
(Reporter)

Comment 12

4 years ago
(In reply to cbazoian12 from comment #11)
> I was using the link
> http://jungels.net/articles/diff-patch-ten-minutes.html. But I believe I
> followed the instructions wrong. I'll go back and try again.

Since firefox development uses mercurial, you'll want to generate your patches using mercurial itself (rather than using the diff and patch commands). This can be done using the "hg export" command.

In order to ensure mercurial is configured properly though you'll want to first run "./mach mercurial-setup" inside the repository clone.

Comment 13

4 years ago
(In reply to Steven MacLeod [:smacleod] from comment #12)
> (In reply to cbazoian12 from comment #11)
> > I was using the link
> > http://jungels.net/articles/diff-patch-ten-minutes.html. But I believe I
> > followed the instructions wrong. I'll go back and try again.
> 
> Since firefox development uses mercurial, you'll want to generate your
> patches using mercurial itself (rather than using the diff and patch
> commands). This can be done using the "hg export" command.
> 
> In order to ensure mercurial is configured properly though you'll want to
> first run "./mach mercurial-setup" inside the repository clone.

So the problem I ran into is that I already made the changes to the files before starting the process of making a diff file. How do I roll back those changes so that I can make a diff file?
(Reporter)

Updated

4 years ago
Flags: needinfo?(smacleod)
(Reporter)

Comment 14

4 years ago
(In reply to cbazoian12 from comment #13)
> So the problem I ran into is that I already made the changes to the files
> before starting the process of making a diff file. How do I roll back those
> changes so that I can make a diff file?

You actually won't need to rollback your changes in order to create the diff file. Assuming you've made your changes inside of your clone of the repository, you should be able to run "hg diff" to show you what has changed.

You can then use "hg commit" to save this changes as a commit, and then export the whole commit to a patch file using "hg export", or upload it to bugzilla using bzexport (which should have been installed by ./mach mercurial-steup).

It is probably worth familiarizing yourself with mercurial. The mercurial project's website[1] is a great resource for learning the basics.

[1] http://mercurial.selenic.com/guide
Flags: needinfo?(smacleod)

Comment 15

4 years ago
This is my first time making a diff file. I have pulled changes since I made my own changes to the code. When I run the "hg diff" command, the terminal spits out a bunch of code I don't recognize. As I understand, I am supposed to just see the changes I made. Is that because I pulled changes since I made my own changes?

Updated

4 years ago
Flags: needinfo?(smacleod)
(Reporter)

Comment 16

4 years ago
(In reply to cbazoian12 from comment #15)
> This is my first time making a diff file. I have pulled changes since I made
> my own changes to the code. When I run the "hg diff" command, the terminal
> spits out a bunch of code I don't recognize. As I understand, I am supposed
> to just see the changes I made. Is that because I pulled changes since I
> made my own changes?

hg diff should only be displaying the changes to your working directory and wouldn't include changes that have been pulled from the server (they will be present in commits)

For example, you were based on commit 'A' from the server
> ... <- A <- Your work

After pulling the result would be
> ... <- A <- Your work
>         \
>          \- Commits pulled <- New default head from server

If you run 'hg diff' without arguments and you are seeing the changes from the commits that came from the server something strange has happened.

So a few questions for you:
 - How exactly did you pull the new changes?
 - Has your work been committed or is it still just part of the working directory?

In the case where you pull to get new changes and things work as expected, you'd then want to "rebase" your work onto the new changes. This is done with the rebase extension[1]

It would be really helpful if we could chat synchronously so I can help you through these mercurial issues more quickly. When you have some time could you join the mozilla irc server[2] and message me (I'm smacleod on irc)?

[1] http://mercurial.selenic.com/wiki/RebaseExtension
[2] https://wiki.mozilla.org/IRC
Flags: needinfo?(smacleod) → needinfo?(cbazoian12)

Comment 17

4 years ago
Thank you. I am working on the patch.
Flags: needinfo?(cbazoian12)

Comment 18

4 years ago
Posted patch bug1132556.diff (obsolete) — Splinter Review
I hope this patch turned out right this time.
Attachment #8569437 - Attachment is obsolete: true
Flags: needinfo?(smacleod)
(Reporter)

Comment 19

4 years ago
(In reply to cbazoian12 from comment #18)
> Created attachment 8577444 [details] [diff] [review]
> bug1132556.diff
> 
> I hope this patch turned out right this time.
Hey sorry for the delay in the response, I had written a reply but I guess I didn't submit it.

Unfortunately it looks like you have many modifications to unrelated files included in the patch. Something you can do before submitting is to open up the diff file in a text editor and check that it contains what you expect.

I'm unsure how exactly the unrelated changes were included into your working directory and then committed. We'll need to create a new commit though which doesn't include the unrelated changes.

It would be really helpful if you could contact me on irc and we could work through this together synchronously.
Flags: needinfo?(smacleod)

Comment 20

3 years ago
Has this bug been fixed ?
(Reporter)

Comment 21

3 years ago
(In reply to faris18omar from comment #20)
> Has this bug been fixed ?

It has not.
Assignee: cbazoian12 → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 22

3 years ago
Steven, since this bug appears to be unclaimed would you mind assigning it to me? I took a look to check it out and believe I have the necessary changes made. Don't want to submit a patch out of turn though!
AJ: Go for it.
Assignee: nobody → ajkerrigan
(Reporter)

Comment 24

3 years ago
(In reply to AJ Kerrigan from comment #22)
> Steven, since this bug appears to be unclaimed would you mind assigning it
> to me? I took a look to check it out and believe I have the necessary
> changes made. Don't want to submit a patch out of turn though!

As :mhoye indicated, please submit your patch, it would be most welcome :)
(Assignee)

Comment 26

3 years ago
This is my first attempt at submitting a patch, so please don't be shy if I'm doing something messed up :). As supporting documentation, here is the before/after mochitest output for the sessionrestore component.

Before:

Browser Chrome Test Summary
        Passed: 1029
        Failed: 0
        Todo: 0
*** End BrowserChrome Test Results ***
SUITE-END | took 207s

After:

Browser Chrome Test Summary
        Passed: 1029
        Failed: 0
        Todo: 0
*** End BrowserChrome Test Results ***
SUITE-END | took 205s
(Reporter)

Comment 27

3 years ago
Comment on attachment 8705277 [details] [diff] [review]
Replace runInContent() with ContentTask.spawn()

In the future when posting patches you'll want to set a review? flag on who you would like to review the patch. When attempting to do that their is a suggested reviewers feature in bugzilla that can provide options for you if you don't know who should review the patch (if it picks the wrong person they'll be able to help decide on someone better).

I'm going to set the flag on myself and I should have a review for you sometime soon. Thanks for the patch! :)
Attachment #8705277 - Flags: review?(smacleod)
(Assignee)

Comment 28

3 years ago
Apologies for the double-patch. I initially missed that the "ss-test:run"
message was only coming from the now-removed runInContent(). To remove
runInContent() more cleanly, I've taken out the "ss-test:run" listener.
Test results are unchanged.
(Assignee)

Updated

3 years ago
Attachment #8705277 - Attachment is obsolete: true
Attachment #8705277 - Flags: review?(smacleod)
(Assignee)

Comment 29

3 years ago
(In reply to Steven MacLeod [:smacleod] from comment #27)
> Comment on attachment 8705277 [details] [diff] [review]
> Replace runInContent() with ContentTask.spawn()
> 
> In the future when posting patches you'll want to set a review? flag on who
> you would like to review the patch. When attempting to do that their is a
> suggested reviewers feature in bugzilla that can provide options for you if
> you don't know who should review the patch (if it picks the wrong person
> they'll be able to help decide on someone better).
> 
> I'm going to set the flag on myself and I should have a review for you
> sometime soon. Thanks for the patch! :)

Oh thanks for the advice! I'll keep that in mind going forward.
(Assignee)

Updated

3 years ago
Attachment #8705288 - Flags: review?(smacleod)
(Reporter)

Comment 30

3 years ago
Comment on attachment 8705288 [details] [diff] [review]
Replace runInContent() with ContentTask.spawn()

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

This looks great, nice first patch :D

I've pushed this to try, which will run the tests and show the results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c5b469bdd43

If all looks good we should be able to land this. You can read about the try server if you'd like: https://wiki.mozilla.org/ReleaseEngineering/TryServer
Attachment #8705288 - Flags: review?(smacleod) → review+
(Assignee)

Comment 31

3 years ago
(In reply to Steven MacLeod [:smacleod] from comment #30)
> Comment on attachment 8705288 [details] [diff] [review]
> Replace runInContent() with ContentTask.spawn()
> 
> Review of attachment 8705288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, nice first patch :D
> 
> I've pushed this to try, which will run the tests and show the results here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c5b469bdd43
> 
> If all looks good we should be able to land this. You can read about the try
> server if you'd like: https://wiki.mozilla.org/ReleaseEngineering/TryServer

Nice! Thanks for the review, guidance and links.
(Reporter)

Comment 32

3 years ago
The try run looks good, marking checkin-needed so someone will land this. Thanks!
Keywords: checkin-needed
(Reporter)

Updated

3 years ago
Attachment #8577444 - Attachment is obsolete: true

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b75960b9d195
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Wow, that was fast. 

AJ, we don't see a lot of easy slam dunks like this from new contributors, and it's a joy to watch this bug happen. Do you have something you'd like to work on next? We have a few of this grade that could use your attention, but we'd be pretty happy to work with you on something a few rungs further up the challenge ladder if you're so inclined.
(Assignee)

Comment 36

3 years ago
(In reply to Mike Hoye [:mhoye] from comment #35)
> Wow, that was fast. 
> 
> AJ, we don't see a lot of easy slam dunks like this from new contributors,
> and it's a joy to watch this bug happen. Do you have something you'd like to
> work on next? We have a few of this grade that could use your attention, but
> we'd be pretty happy to work with you on something a few rungs further up
> the challenge ladder if you're so inclined.

Thanks Mike, very kind of you to say and I'd love to work with one of you. I'm happy to try something a bit more challenging, though I don't want to overreach and end up wasting people's time either. Would it be useful to chat a bit either via email or IRC (ajkerrigan)?
(Assignee)

Updated

3 years ago
Flags: needinfo?(mhoye)
I'll email you directly!
Flags: needinfo?(mhoye)

Comment 38

3 years ago
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
STR: Developer specific testing.

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64
You need to log in before you can comment on or make changes to this bug.