Closed Bug 1555310 Opened 5 years ago Closed 5 years ago

Remove unused variables from MockProvider (remains of deleted _delayCallback)

Categories

(Toolkit :: Add-ons Manager, task, P3)

Product:
Toolkit
Component:
Add-ons Manager
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: robwu, Assigned: abowler, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

Bug 1555310 : Removed all instances of redundant variables callbackTimers and timerLocations from toolkit/mozapps/extentions/test/browser/head.js
47 bytes, text/x-phabricator-request
Details | Review
Bug 1555310 - remove unused variables in MockProvider r=robwu
47 bytes, text/x-phabricator-request
Details | Review

In https://hg.mozilla.org/mozilla-central/rev/112ff1e321a , the _delayCallback method was removed from toolkit/mozapps/extensions/test/browser/head.js.

The callbackTimers and timerLocations member variables are no longer used, and should be removed, together with https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/toolkit/mozapps/extensions/test/browser/head.js#844-860

Flags: qe-verify-

This is a good first bug, and can be fixed by removing the code that was referenced in comment 0, including the declarations of the variables in the same file.

To get started, set up a development environment following the instructions at:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Mentor: rob
Keywords: good-first-bug
Priority: -- → P3

Hello Rob, I am new to Mozilla open source. I would like to fix this bug!

Hi Chaitanya, welcome!

The first two comments explain what needs to be done to resolve this bug. To get started, set up a development environment. Once you have written a patch, submit the patch. Feel free to message me if you have any questions.

Hey Rob,
I've followed all instructions in Setup guide, but I seem to be stuck at the final step.
I ran ./mach bootstrap which completed successfully.
Now I cannot find ./mach or build anywhere. Also the instructions run out here and I do not how to proceed.
Any help would be appreciated

Which operating system are you using? From the slashes I guess that you're using macOS or Linux, but correct me if I'm wrong.

Since you've successfully ran ./mach bootstrap before, it is obvious that you have a copy of the mozilla-central repository.
If ./mach build does not work any more, then your current directory is not mozilla-central, so you need to change the directory back to mozilla-central.
When you know that you did really create mozilla-central, but forgot about the location, then you can use the find command to locate it again. E.g. find / -type d -name mozilla-central.

Sorry I forgot to mention I'm running Linux on my system.
I tried running ./mach build after changing directory to /mozilla-central
The script is running now!

Just a quick update, the make script has done the job. I will get to fixing the bug now!

Hey Rob!
I have followed the steps and removed all instances of 'callbackTimers' and 'timerLocations' and also edited the 'MP_shutdown()' function.
I am trying to './mach build' but my laptop (6GB ram and enough free space) froze. I forced shutdown and ran './mach busted' which showed me a few bugs. Problem is I cannot find any support on how to fix these bugs
'''
Bug 1552120 - Deleted search extensions are not rebuilt
Bug 1557030 - "ERROR: Could not find libclang to generate rust bindings for C/C++"
Bug 1557157 - mach wpt - ValueError: Expecting : delimiter: line 9001 column 75 (char 765061)
Bug 1556371 - |mach doc | command is not working
Bug 1541424 - mach try fuzzy <path> is ignoring skip-if and disabled in mochitest.ini
Bug 1554987 - Invalid remote name: "hg::ssh://hg.mozilla.org/try" error running ./mach try (fuzzy|chooser) on a git repo
Bug 1487552 - Provide further steps to devs for xcode-select --install on Mojave (AKA 'inttypes.h' file not found)
Bug 1500924 - Packaging fennec is broken until build is clobbered
Bug 1543447 - Multiple minutes to dump a stack from NS_ASSERTION in mochitests and reftests on OS X
Bug 1548948 - |mach install| fails to start x86 emulator
Bug 1522931 - OSX SDK version check doesn't work when using default SDK
Bug 1547040 - --try-test-paths isn't respected anymore
Bug 1547269 - Don't put LLVM in $PATH since we now prefer .mozbuild
Bug 1544295 - Local linux debug build fails to run (GLIBCXX_3.4.22 not found)
'''
Is there a fix for this?

The only time when I experienced a frozen system when compiling (not sure if it was Firefox) was when I ran out of memory.
Without other details, I would still guess that running out of memory is the most likely condition.

With artifact builds, the system requirements (and time to build!) are significantly lower. So unless you want to make change C++ code, I suggest that you enable artifact builds instead, following the instructions at: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation#Let%E2%80%99s_Build_Firefox

Hey Rob!
Sorry I couldn't get back to you earlier. I have been reading up on Arcanist and Phabricator to understand the code review process.
But I am still uncertain as to what tests I must run before submitting my code for review. Also since you are the mentor on this bug, should I assign you as a reviewer or the module owner (Who I think is Narcis Beleuzu) ?
Thanks a lot

Just a quick update,
I ran mach build with artifact build enabled in my .mozconfig and it built succesfully on the second try. It didn't hang my system this time and was done much faster. After that I followed the link https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/So_you_just_built_Firefox for further steps. I am now running the xpcshell-test script.

Hey Rob!
So I ran mach test in the relevant folder (/mozilla-central/toolkit/mozapps/extensions/test/browser)
The overall summary : "Ran 6950 checks (6869 subtests, 81 tests) Expected results: 6942 Skipped: 5 tests Unexpected results: 3 subtest: 3 (3 fail) "

I am debugging the code now, there seem to be some issues with window closing which may be related to the MP_shutdown method

I don't expect any failures from removing the code. To see whether the issue is related to your change at all, you could temporarily revert your changes and see if the test still fails.

Since this is test-only code, I would look up the tests that use MockProvider and run those to check whether everything still works as expected. Or conversely, you could look at the list of failing tests and check whether they contain MockProvider.

Hey Rob!
You were right, when I run mach test on the original code with all instances of callbackTimers and timerLocations also returns the same failed tests. I have committed to changeset 476922 with the commit message including the bug id.

Do I need to submit it for review to you? Or is someone else responsible for the review? I have already read the docs on Phabricator and Arcanist, so it shouldn't take me long

I will be providing the first round of review. Once that looks right, I will find you a module peer to sign off on your patch.

Once you've created a Phabricator account and uploaded your patch, just add me as a reviewer.
(you've probably already seen https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch )

Assignee: nobody → chaitannah
Status: NEW → ASSIGNED

Before this change unused variables callbackTimers and timerLocations were included in head.js, which were remains of removed method _delayCallback. The function MP_shutdown is now simplified as _delayCallback is removed.

I have added you as reviewer from username robwu
I would be happy to do anything else as required

Hey Chaitanya, how's it going with this bug?

Hey Chaitanya, since we haven't heard from you in awhile we're going to re-open this bug to other contributors. If you'd like to continue working on it, just let us know!

Assignee: chaitannah → nobody
Status: ASSIGNED → NEW

I would like to take on this bug if Chaitanya is not able to continue with it.

Can you please clarify if you would need the original changes to be done again or if there is a way to build off of Chaitanya's work.

Hey abowler2! If you go to the top of the bug you can see an attachment that links to Chaitanya's patch. You can take a look at the diff and also Rob's comments. :)

Rob's currently attending an event for the rest of the week, but if you want to submit a patch in Phabricator, he should be able to review it when he gets back next week.

Happy bug fixing!

Thank you.

I will go over what was worked on previously and Rob's comments and submit my patch once done.

Before this change / _delayCallback was removed. As a result the variables callbackTimers, timerLocations, and useAsyncCallback were no longer needed.

Assignee: nobody → abowler2
Status: NEW → ASSIGNED
Attachment #9094075 - Attachment description: Bug 1555310 - removal of unused variables in MockProvider r=robwu → Bug 1555310 - remove unused variables in MockProvider r=robwu

Thanks for your feedback Rob.

checkin-needed

Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2340335f5458
remove unused variables in MockProvider r=robwu

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/2340335f5458

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Thanks so much for the patch, abowler2! 🎉 Your Mozillians profile has been vouched for and your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Welcome onboard! We look forward to seeing you around.

Thank you!! I'm looking forward to continued contributions.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: