896688 - Remove state machine from nsSearchService initialization

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Steven MacLeod [:smacleod]]

Bug 887868 simplifies the nsSearchService initialization state machine to only two states. We can probably simplify things further to use just a flag.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Hey Steven, can you provide a bit more detail here about what needs to be done?

Comment 2

[Steven MacLeod [:smacleod]]

Updated the user story to hold instructions on how to fix this bug.

Comment 3

[Roy Li]

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

Comment 4

[Steven MacLeod [:smacleod]]

(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.

Comment 5

[Roy Li]

Attachment: 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!

Comment 6

[Steven MacLeod [:smacleod]]

(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.

Comment 7

[Steven MacLeod [:smacleod]]

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 :)

Comment 8

[Roy Li]

Attachment: Changed finite state machine to flag.

Addressed your comments.

Comment 9

[Steven MacLeod [:smacleod]]

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 :)

Comment 10

[Roy Li]

Attachment: Replace state machine in nsSearchService initialization with flag

Okay, I think I merged the patches correctly.

Comment 11

[Steven MacLeod [:smacleod]]

(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!

Comment 12

[Steven MacLeod [:smacleod]]

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.

Comment 13

[Steven MacLeod [:smacleod]]

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.

Comment 14

[Steven MacLeod [:smacleod]]

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

Comment 15

[Roy Li]

Attachment: 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?

Comment 16

[Steven MacLeod [:smacleod]]

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

Comment 17

[Roy Li]

Attachment: changed patch commit message

Here's the patch with the new commit message.

Comment 18

[Steven MacLeod [:smacleod]]

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

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

Excellent, thanks for updating it!

Comment 19

[Carsten Book [:Tomcat]]

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!

Comment 20

[Steven MacLeod [:smacleod]]

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

Comment 21

[Roy Li]

Attachment: Replace finite state machine with flag in nsSearchService

Comment 22

[Steven MacLeod [:smacleod]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1c2a91bccd4a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c2a91bccd4a