Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Description • 12 years ago

The event listeners in the b2g/components/SignInToWebsite pipe object are not getting cleaned up properly because the pipe is not receiving the "finished" message from identity.js.  This is causing the event callbacks to fire two, three, four, etc. times per message.

It appears that once gaia shuts down the identity.js window, no callbacks fire, so the close handlers don't get a chance to tell gecko to clean up.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 1 • 12 years ago

Clean up multiple listeners by sending a finished message to gecko first, and then messaging gaia.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 2 • 12 years ago

Comment on attachment 676742 [details] [diff] [review]
clean up message listeners before closing window

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

This fixes a problem that emerged when we started testing against the trusty ui.  There may have been a race condition all along, but it wasn't evident to me with the regular popup

	Ben Adida [:benadida]
	Comment 3 • 12 years ago

memory leak, marking blocker.

	Ben Adida [:benadida]
	Comment 4 • 12 years ago

Comment on attachment 676742 [details] [diff] [review]
clean up message listeners before closing window

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

r+ with tiny comment nit below.

::: b2g/chrome/content/identity.js
@@ +62,2 @@
>  /*
>   * Notify the UI to close the dialog and return to the caller application

if you're removing the callback, change this comment.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 5 • 12 years ago

changed comment per Comment #4 r=benadida

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 12 years ago

This doesn't apply cleanly to m-c. Please rebase.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 7 • 12 years ago

New patch fixes collision with m-c

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 8 • 12 years ago

(In reply to Ryan VanderMeulen from comment #6)
> This doesn't apply cleanly to m-c. Please rebase.

Thanks, Ryan.  Should be good to go, now.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 9 • 12 years ago

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=c2ebceab46ab

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7914709279

Should this have a test?

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/0e7914709279

	Jason Smith [:jsmith]
	Comment 12 • 12 years ago

I take it the right way to verify this is to go through the persona login flows and make sure I'm not seeing multiple unexpected events getting fired, right?

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea28d6b8dbd8

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 14 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #12)
> I take it the right way to verify this is to go through the persona login
> flows and make sure I'm not seeing multiple unexpected events getting fired,
> right?

Yes.  We're new to marionette tests; once we've got them figured out, there will be a test for this.