Edgar Chen [:edgar] Assignee
	Description • 11 years ago

Have a quick debug for this, it seems parent side (SpecialPowersObserverAPI) doesn't receive IPC message sent by child side (SpecialPowersAPI).

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 1 • 10 years ago

This was suggested to be looked at during the marionette meeting, so should this be a P1?

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 2 • 10 years ago

Is this bug blocking anything, Edgar?

	Edgar Chen [:edgar] Assignee
	Comment 3 • 10 years ago

(In reply to Malini Das [:mdas] from comment #2)
> Is this bug blocking anything, Edgar?

Yes, we would like to enable OOP mode for telephony related test case in bug 926277. But somehow SpecialPower.*Permission doesn't work in OOP mode, so the test case replying on `pushPermissions`, `pushFooPrefs`, ... will fail. :(

	Edgar Chen [:edgar] Assignee
	Comment 4 • 10 years ago

In OOP mode, marionette switches frame to test-container, but specialpowers doesn't register listener to the corresponding messageManager.

	Edgar Chen [:edgar] Assignee
	Comment 5 • 10 years ago

Hi mdas, I am not sure if this is a good way to fix this, may I have your feedback? Thank you. :)

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 6 • 10 years ago

I'm looking at the marionette patch, but the changes to SpecialPowersObserver and b2g_start_script should be looked at by someone else, since I'm not familiar with them. I'm suggesting jmaher and ahal respectively.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 7 • 10 years ago

Comment on attachment 8495880 [details] [diff] [review]
Patch, v1

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

nothing scares me here- a few questions.

::: testing/marionette/marionette-frame-manager.js
@@ +142,5 @@
>      remoteFrames.push(aFrame);
>      this.currentRemoteFrame = aFrame;
> +
> +    aFrame.specialPowersObserver = new specialpowers.SpecialPowersObserver();
> +    aFrame.specialPowersObserver.init(mm);

on desktop there are so many cases to load the observer, is this going to make it available to all cases in marionette?

::: testing/mochitest/b2g_start_script.js
@@ -81,5 @@
> -  mm.addMessageListener("SpecialPowers.Quit", specialPowersObserver);
> -  mm.addMessageListener("SpecialPowers.Focus", specialPowersObserver);
> -  mm.addMessageListener("SPPermissionManager", specialPowersObserver);
> -  mm.addMessageListener("SPLoadChromeScript", specialPowersObserver);
> -  mm.addMessageListener("SPChromeScriptMessage", specialPowersObserver);

in the new way, we are adding in:
 this._messageManager.addMessageListener("SPWebAppService", this);	
this._messageManager.addMessageListener("SPObserverService", this);	

is that problematic?

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 8 • 10 years ago

You're on the right track with the marionette-frame-manager.js changes. There is one issue; we don't uninitialize and remove the specialPowersObservers upon session deletion, so when we create a new session, we create a new observer even though one already exists.

We should remove all observers when our session is closed to avoid leaking memory. I've added a patch that you can use to amend yours, and it will remove the observers when we end our session.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 9 • 10 years ago

Comment on attachment 8495880 [details] [diff] [review]
Patch, v1

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

f+ if you add my patch (only f+ing marionette changes)

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 10 • 10 years ago

(In reply to Joel Maher (:jmaher) from comment #7)
> @@ +142,5 @@
> >      remoteFrames.push(aFrame);
> >      this.currentRemoteFrame = aFrame;
> > +
> > +    aFrame.specialPowersObserver = new specialpowers.SpecialPowersObserver();
> > +    aFrame.specialPowersObserver.init(mm);
> 
> on desktop there are so many cases to load the observer, is this going to
> make it available to all cases in marionette?

Not all cases, but it will be initialized for all out of process frames we switch into.

	Edgar Chen [:edgar] Assignee
	Comment 11 • 10 years ago

(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 8495880 [details] [diff] [review]
> Patch, v1
>
> ::: testing/mochitest/b2g_start_script.js
> @@ -81,5 @@
> > -  mm.addMessageListener("SpecialPowers.Quit", specialPowersObserver);
> > -  mm.addMessageListener("SpecialPowers.Focus", specialPowersObserver);
> > -  mm.addMessageListener("SPPermissionManager", specialPowersObserver);
> > -  mm.addMessageListener("SPLoadChromeScript", specialPowersObserver);
> > -  mm.addMessageListener("SPChromeScriptMessage", specialPowersObserver);
> 
> in the new way, we are adding in:
>  this._messageManager.addMessageListener("SPWebAppService", this);	
> this._messageManager.addMessageListener("SPObserverService", this);	
> 
> is that problematic?

These two messages are for SpecialPowers's API, setAllAppsLaunchable() and notifyObserversInParentProcess(). I think it is fine to add them unless some tests call theses API and expect they do nothing. Thank you.

	Edgar Chen [:edgar] Assignee
	Comment 12 • 10 years ago

(In reply to Malini Das [:mdas] from comment #9)
> Comment on attachment 8495880 [details] [diff] [review]
> Patch, v1
> 
> Review of attachment 8495880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ if you add my patch (only f+ing marionette changes)

Thank you, mdas. I will provide a new patch includes your patch.

	Edgar Chen [:edgar] Assignee
	Comment 13 • 10 years ago

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

test_notification_resend.html fails due to time out. I have no idea why it fails, still under investigating.

05:23:16     INFO -  581 INFO TEST-START | /tests/dom/tests/mochitest/notification/test_notification_resend.html
05:28:38     INFO -  dumping last 1 message(s)
05:28:38     INFO -  if you need more context, please use SimpleTest.requestCompleteLog() in your test
05:28:38     INFO -  582 INFO ::Notification Tests:: | Set manifestURL
05:28:38     INFO -  583 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/notification/test_notification_resend.html | Test timed out. - expected PASS
05:28:39     INFO -  584 INFO TEST-OK | /tests/dom/tests/mochitest/notification/test_notification_resend.html | took 322631ms

	Edgar Chen [:edgar] Assignee
	Comment 14 • 10 years ago

test_notification_resend.html needs apps_launchable to be true, but apps_launchable was flushed by previous test when it calls SimpleTest.finish()[1]. So we should enable apps_launchable to true before starting the test. We don't encounter this problem now is because the flushAllAppsLaunchable() doesn't work at all [2].

[1] http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#858
[2] b2g_start_script.js miss adding listener for "SPWebAppService".

	Edgar Chen [:edgar] Assignee
	Comment 15 • 10 years ago

- Rebase
- Merge the patch provided by mdas.
- Enable marionette tests for SpecialPowers in test_container (bug 1060061).
- Address comment #14.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 16 • 10 years ago

Comment on attachment 8497555 [details] [diff] [review]
Patch, v2

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

r- due to a bug I just noticed

::: testing/marionette/marionette-frame-manager.js
@@ +142,5 @@
>      remoteFrames.push(aFrame);
>      this.currentRemoteFrame = aFrame;
> +
> +    aFrame.specialPowersObserver = new specialpowers.SpecialPowersObserver();
> +    aFrame.specialPowersObserver.init(mm);

Ah, I was testing this locally, and I noticed that we don't reattach the specialPowersObserver when we restart our session.

Since we remove the specialPowersObserver in removeSpecialPowers function when we delete our session, then if we restart a session and navigate to a frame that previously had specialPowers in it, then that observer won't be present, because we don't go through this same code path. So, when we revisit a previously seen frame, we should check if it has an observer. If it doesn't, we can initialize one.

Can you add something like:

if (!frame.specialPowersObserver) {
  frame.specialPowersObserver = new specialpowers.SpecialPowersObserver();
  frame.specialPowersObserver.init(mm);
}

after this line https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-frame-manager.js#123?

	Edgar Chen [:edgar] Assignee
	Comment 17 • 10 years ago

(In reply to Malini Das [:mdas] from comment #16)
> Comment on attachment 8497555 [details] [diff] [review]
> Patch, v2
> 
> Review of attachment 8497555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- due to a bug I just noticed
> 
> ::: testing/marionette/marionette-frame-manager.js
> @@ +142,5 @@
> >      remoteFrames.push(aFrame);
> >      this.currentRemoteFrame = aFrame;
> > +
> > +    aFrame.specialPowersObserver = new specialpowers.SpecialPowersObserver();
> > +    aFrame.specialPowersObserver.init(mm);
> 
> Ah, I was testing this locally, and I noticed that we don't reattach the
> specialPowersObserver when we restart our session.
> 
> Since we remove the specialPowersObserver in removeSpecialPowers function
> when we delete our session, then if we restart a session and navigate to a
> frame that previously had specialPowers in it, then that observer won't be
> present, because we don't go through this same code path. So, when we
> revisit a previously seen frame, we should check if it has an observer. If
> it doesn't, we can initialize one.
> 
> Can you add something like:
> 
> if (!frame.specialPowersObserver) {
>   frame.specialPowersObserver = new specialpowers.SpecialPowersObserver();
>   frame.specialPowersObserver.init(mm);
> }
> 
> after this line
> https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> frame-manager.js#123?

Will provide a new version to address this, thank you.

	Edgar Chen [:edgar] Assignee
	Comment 18 • 10 years ago

Address review comment #16.

	Edgar Chen [:edgar] Assignee
	Comment 20 • 10 years ago

Try looks good so far: https://tbpl.mozilla.org/?tree=Try&rev=db39b9a2948a

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 21 • 10 years ago

Comment on attachment 8498010 [details] [diff] [review]
Patch, v3

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

Tested locally and the marionette changes look perfect, thanks!

R+, but the only issue I have with the patch as a whole is that it's bitrotted, so we can't land it as is. Can you upload a new version of this patch that applies cleanly to mozilla-central?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 22 • 10 years ago

Comment on attachment 8498010 [details] [diff] [review]
Patch, v3

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

a few comments, no need to require them prior to landing, but please do address them in due time.

::: testing/marionette/marionette-frame-manager.js
@@ +167,5 @@
>    /**
> +   * This function removes any SpecialPowersObservers from OOP frames.
> +   */
> +  removeSpecialPowers: function FM_removeSpecialPowers() {
> +    for (let i = 0; i < remoteFrames.length; i++) {

where is remoteFrames defined?  If it is a global, I would like to know that based on the variable name

::: testing/marionette/marionette-server.js
@@ +2214,5 @@
>        let winEnum = this.getWinEnumerator();
>        while (winEnum.hasMoreElements()) {
>          winEnum.getNext().messageManager.removeDelayedFrameScript(FRAME_SCRIPT);
>        }
> +      this.curBrowser.frameManager.removeSpecialPowers();

is this the only spot we need to call this?

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 23 • 10 years ago

(In reply to Joel Maher (:jmaher) from comment #22)
> Comment on attachment 8498010 [details] [diff] [review]
> Patch, v3
> 
> Review of attachment 8498010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few comments, no need to require them prior to landing, but please do
> address them in due time.
> 
> ::: testing/marionette/marionette-frame-manager.js
> @@ +167,5 @@
> >    /**
> > +   * This function removes any SpecialPowersObservers from OOP frames.
> > +   */
> > +  removeSpecialPowers: function FM_removeSpecialPowers() {
> > +    for (let i = 0; i < remoteFrames.length; i++) {
> 
> where is remoteFrames defined?  If it is a global, I would like to know that
> based on the variable name
it is a global variable: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-frame-manager.js#17
> 
> ::: testing/marionette/marionette-server.js
> @@ +2214,5 @@
> >        let winEnum = this.getWinEnumerator();
> >        while (winEnum.hasMoreElements()) {
> >          winEnum.getNext().messageManager.removeDelayedFrameScript(FRAME_SCRIPT);
> >        }
> > +      this.curBrowser.frameManager.removeSpecialPowers();
> 
> is this the only spot we need to call this?

yes, this is called when delete_session() is explicitly called, and when the socket closes (ie: client disconnects)

	Edgar Chen [:edgar] Assignee
	Comment 24 • 10 years ago

Thanks for the review, mdas and jmaher.

https://hg.mozilla.org/integration/b2g-inbound/rev/36107e095754

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

https://hg.mozilla.org/mozilla-central/rev/36107e095754