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

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

Is this bug blocking anything, Edgar?

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

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

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

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.

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?

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.

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)

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

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

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

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

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

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

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?

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

Address review comment #16.

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

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?

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?

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

Thanks for the review, mdas and jmaher. https://hg.mozilla.org/integration/b2g-inbound/rev/36107e095754