Remove state machine from nsSearchService initialization

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: smacleod, Assigned: li.roy.young, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 38
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [lang=js][good first bug])

User Story

This bug primarily deals with switching a flag variable containing two possible states into a boolean flag and should be mostly straightforward.

The `engineMetadataService`[1] component of the nsSearchService uses a state machine during initialization. The current state of initialization is stored in '_initState'[2].

Since the patch in Bug 887868 simplified initialization there are now only 2 states[3], 'NOT_STARTED' and 'FINISHED_SUCCESS'. To simplify initialization even further, these two states could be replaced with a single boolean flag (e.g. 'initialized'), which would be set to `true` when initialization completed.

After replacing '_initState'[2] with the new flag, each use of the old variable[4] will need to be updated. Cases dealing with the 'NOT_STARTED'[5] case would be replaced with 'false' and 'FINISHED_SUCCESS'[5] would be replaced with 'true'.

Among other things this will simplify the `switch (this._initState) {`[7] blocks into simple `if (!this._initialized) {` blocks. Overall the code should be simpler and more readable after this refactoring.


[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=1f51eb900e57#4761
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=1f51eb900e57#4786
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=1f51eb900e57#4774


[4] http://mxr.mozilla.org/mozilla-central/search?string=_InitState&find=nsSearchService.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[5] http://mxr.mozilla.org/mozilla-central/search?string=NOT_STARTED&find=nsSearchService.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[6] http://mxr.mozilla.org/mozilla-central/search?string=FINISHED_SUCCESS&find=nsSearchService.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

[7] http://mxr.mozilla.org/mozilla-central/search?string=switch+%28this._initState%29+{&find=nsSearchService.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Bug 887868 simplifies the nsSearchService initialization state machine to only two states. We can probably simplify things further to use just a flag.
(Reporter)

Updated

6 years ago
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Assignee: smacleod → nobody
Whiteboard: [mentor=smacleod][lang=js]
(Reporter)

Updated

5 years ago
Status: ASSIGNED → NEW
Flags: firefox-backlog+
Mentor: smacleod
Whiteboard: [mentor=smacleod][lang=js] → [lang=js]
Hey Steven, can you provide a bit more detail here about what needs to be done?
Flags: needinfo?(smacleod)
(Reporter)

Comment 2

4 years ago
Updated the user story to hold instructions on how to fix this bug.
Points: --- → 2
User Story: (updated)
Flags: needinfo?(smacleod)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [lang=js] → [lang=js][good first bug]
(Assignee)

Comment 3

4 years ago
Hi, I believe I fixed this bug. I've never contributed before; can someone please guide me through the next step in the process?
(Reporter)

Comment 4

4 years ago
(In reply to Roy Li from comment #3)
> Hi, I believe I fixed this bug. I've never contributed before; can someone
> please guide me through the next step in the process?

Hi Roy. That's excellent!

I've assigned the bug to you and will work with you through the steps. https://developer.mozilla.org/en/docs/Introduction has a good overview of all the steps you need to go through to contribute. Since you believe you've fixed the bug I'll assume you have built firefox already (if not, that should be your first step).

Now that you've fixed this, you'll need to attach your code to the bug and get review. You'll want to either attach a patch file or use "bzexport" to upload your code. You can find instructions for how to do that at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Once the code is uploaded you'll need to ask for review - please mark me as the reviewer and I will take a look at the code.

Please let me know if you have any more questions or would like more help with any of the steps.
Assignee: nobody → li.roy.young
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
Created attachment 8557810 [details] [diff] [review]
Changed finite state machine to flag.

Hi Steven, I'm typing this comment in bzexport (I assume it will post it as it uploads the patch). Hope this works and thanks for the help!
Attachment #8557810 - Flags: review?(smacleod)
(Reporter)

Comment 6

4 years ago
(In reply to Roy Li from comment #5)
> Created attachment 8557810 [details] [diff] [review]
> Changed finite state machine to flag.
> 
> Hi Steven, I'm typing this comment in bzexport (I assume it will post it as
> it uploads the patch). Hope this works and thanks for the help!

It did work, thanks for uploading the patch! I will take a look this evening and review it for you.
(Reporter)

Comment 7

4 years ago
Comment on attachment 8557810 [details] [diff] [review]
Changed finite state machine to flag.

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

Roy, this is a great start - I just have a few small comments I would like you to address.

::: toolkit/components/search/nsSearchService.js
@@ +4760,5 @@
>  
>  var engineMetadataService = {
>    _jsonFile: OS.Path.join(OS.Constants.Path.profileDir, "search-metadata.json"),
>  
> +  // Boolean flag that is true if initialization was successful

Please add a period to the end of this comment.

@@ +4761,5 @@
>  var engineMetadataService = {
>    _jsonFile: OS.Path.join(OS.Constants.Path.profileDir, "search-metadata.json"),
>  
> +  // Boolean flag that is true if initialization was successful
> +  _initialized: null,

lets have this set to false instead of null to start.

@@ +4777,5 @@
>        // Launch asynchronous initialization
>        let initializer = this._initializer = Promise.defer();
>        Task.spawn((function task_init() {
>          LOG("metadata init: starting");
> +        if (!this._initialized) {

I think we can add back in the throwing of an error (which used to be the 'default' case) here, and make this conditional a bit cleaner.

We can replace "!this._initialized" with "this._initialized" and then throw in that case. This allows us to have the actual initialization code outside of the conditional.

We should also update the message of the thrown error to make sense in the new code. Something like "metadata init: invalid state, _initialized is true but initialization promise has not been resolved".

So things would look like:
> if (this._initialized) {
>   throw new Error(...) // Error case which was old default case
> }
> 
> // 1. Load json file if it exists
> try {
>   ...

Note the try block above is not in an else case, it is just executed after the check above it.

@@ -4822,5 @@
>              }
> -            break;
> -
> -          default:
> -            throw new Error("metadata init: invalid state " + this._initState);

This 'default' case was not converted to use the new '_initialized' property. It corresponds to when '_initialized == false`. We'll want to have this represented in the new code still - see my other comment about where I think this should be added.

@@ +4829,2 @@
>        return;
> +    } else {

Since the if case above returns, we can just have the else case be executed, we don't need to wrap it in the else block.

@@ -4875,5 @@
> -        this._initState = this._InitStates.FINISHED_SUCCESS;
> -        break;
> -
> -      default:
> -        throw new Error("metadata syncInit: invalid state " + this._initState);

This default case was actually dead code which should have been removed, so we don't need to worry about translating it over like the one I commented above :)
Attachment #8557810 - Flags: review?(smacleod) → feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8559358 [details] [diff] [review]
Changed finite state machine to flag.

Addressed your comments.
Attachment #8559358 - Flags: review?(smacleod)
(Reporter)

Comment 9

4 years ago
Hi Roy, thanks for making the changes so quickly, I took a peek and things are looking great.

Generally when updating a patch with review feedback, you'll want to upload a new patch that contains the entire set of changes from start to end, rather than a new patch that builds on the previous. You'll also want to mark the old patch as "obsolete".

Could you please upload a new patch containing the full difference of your changes and mark the other two as obsolete? Let me know if you have any questions or would like help :)
Flags: needinfo?(li.roy.young)
(Assignee)

Updated

4 years ago
Attachment #8557810 - Attachment is obsolete: true
Flags: needinfo?(li.roy.young)
(Assignee)

Updated

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

Comment 10

4 years ago
Created attachment 8560596 [details] [diff] [review]
Replace state machine in nsSearchService initialization with flag

Okay, I think I merged the patches correctly.
Attachment #8560596 - Flags: review?(smacleod)
(Reporter)

Comment 11

4 years ago
(In reply to Roy Li from comment #10)
> Created attachment 8560596 [details] [diff] [review]
> Replace state machine in nsSearchService initialization with flag
> 
> Okay, I think I merged the patches correctly.

Thanks for folding the patches together, I took a quick peek and it looks good. I'm going to be flying today so I won't be able to give this a full review until tomorrow. Thanks!
(Reporter)

Comment 12

4 years ago
Hey Roy, I didn't have a chance to review this today, sorry! I'll be back at work tomorrow though and will get to it for sure.
(Reporter)

Comment 13

4 years ago
Comment on attachment 8560596 [details] [diff] [review]
Replace state machine in nsSearchService initialization with flag

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

This is looking great, there are just a few small things to fix.

Also, you'll want to be running the tests under the "toolkit/components/search/tests/xpcshell/" directory to make sure everything is working. You can execute these tests from inside the repository by running:
 $ ./mach xpcshell-test toolkit/components/search/tests/xpcshell/

::: toolkit/components/search/nsSearchService.js
@@ +4849,5 @@
>        Task.spawn((function task_init() {
>          LOG("metadata init: starting");
> +        if (this._initialized) {
> +          throw new Error("metadata init: invalid state, _initialized is true 
> +                          but initialization promise has not been resolved");

Let's wrap this long string like so (and remove the trailing whitespace):
>          throw new Error("metadata init: invalid state, _initialized is " +
>                          "true but initialization promise has not been " +
>                          "resolved");

@@ +4870,5 @@
> +          // Couldn't load json, use an empty store
> +          LOG("metadata init: could not load JSON file " + ex);
> +          this._store = {};
> +        }
> +      }

This is an extra "}" which should be removed.

@@ +4872,5 @@
> +          this._store = {};
> +        }
> +      }
> +      this._initialized = true;
> +      LOG("metadata init: complete");

These two lines should be indented 2 spaces.

@@ +4917,2 @@
>      }
> +    this._initialized = true;

Let's add a blank line before this.
Attachment #8560596 - Flags: review?(smacleod) → feedback+
(Reporter)

Comment 14

4 years ago
Also, you'll want to update the commit message to include this bug number, a description, and "r=smacleod" indicating I am the reviewer. There are more details here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
(Assignee)

Updated

4 years ago
Attachment #8560596 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 8562178 [details] [diff] [review]
Replace state machine in nsSearchService initialization with flag

Okay, those things should be fixed. Sorry for making such silly mistakes.

As far as the commit message is concerned, that's only for committing and not for submitting patches for review, correct?
Attachment #8562178 - Flags: review?(smacleod)
(Reporter)

Comment 16

4 years ago
Comment on attachment 8562178 [details] [diff] [review]
Replace state machine in nsSearchService initialization with flag

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

This looks good. Thanks for fixing this up Roy!

I've pushed this to try[1], if that passes we can land the code: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d7b297b9bed

(In reply to Roy Li from comment #15)
> As far as the commit message is concerned, that's only for committing and
> not for submitting patches for review, correct?

Generally you'll want to describe your change in the commit message even while it's just being reviewed. Could you please upload a new patch file with a proper commit message? We'll want that uploaded so that we can land the commit when the tests pass.

Thanks again for working on this, I hope you continue contributing in the future :). Let me know if you'd like any help finding more things to work on.


[1] https://wiki.mozilla.org/Build:TryServer
Attachment #8562178 - Flags: review?(smacleod) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 8562588 [details] [diff] [review]
changed patch commit message

Here's the patch with the new commit message.
(Assignee)

Updated

4 years ago
Attachment #8562178 - Attachment is obsolete: true
(Reporter)

Comment 18

4 years ago
Comment on attachment 8562588 [details] [diff] [review]
changed patch commit message

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

Excellent, thanks for updating it!
Attachment #8562588 - Flags: review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
Hi,

this failed to apply cleanly:

dding 896688 to series file
renamed 896688 -> roy1.patch
applying roy1.patch
patching file toolkit/components/search/nsSearchService.js
Hunk #3 FAILED at 4892
1 out of 4 hunks FAILED -- saving rejects to file toolkit/components/search/nsSearchService.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Steven, Roy: could you take a look, thanks!
Flags: needinfo?(smacleod)
Keywords: checkin-needed
(Reporter)

Comment 20

4 years ago
Hey Roy, it seems that there was another commit[1] touching nsSearchService.js that landed just before we tried to land this. Do you think you could update your patch so it applies cleanly on top of the changes to nsSearchService.js[2]?


[1] http://hg.mozilla.org/mozilla-central/rev/91100de4f2ad
[2] http://hg.mozilla.org/mozilla-central/diff/91100de4f2ad/toolkit/components/search/nsSearchService.js
Flags: needinfo?(smacleod) → needinfo?(li.roy.young)
(Assignee)

Comment 21

4 years ago
Created attachment 8563676 [details] [diff] [review]
Replace finite state machine with flag in nsSearchService
Attachment #8563676 - Flags: review?(smacleod)
(Assignee)

Updated

4 years ago
Attachment #8562588 - Attachment is obsolete: true
(Reporter)

Comment 22

4 years ago
Comment on attachment 8563676 [details] [diff] [review]
Replace finite state machine with flag in nsSearchService

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

Thanks for updating this
Attachment #8563676 - Flags: review?(smacleod) → review+
(Reporter)

Updated

4 years ago
Flags: needinfo?(li.roy.young)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1c2a91bccd4a
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1c2a91bccd4a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
(Reporter)

Updated

4 years ago
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.