I have some patches for making this work.

If a subframe navigates, it can change the value of canGoBack for the top-level frame.

In some cases, the load event listener in a test was triggering before the browser.js listener. It's very important that gMultiProcessBrowser always have the correct value, so we should initialize it as early as possible.

The MozStorageEvent is defined not to bubble, so we should listen with useCapture = true. This happens to work without e10s because the event is dispatched on the exact target we're listening on (the InProcessTabChild). In e10s, we listen on the parent of the target that the event is dispatched on, so we need useCapture to be correct.

Comment on attachment 8479475 [details] [diff] [review] mode-switch Review of attachment 8479475 [details] [diff] [review]: ----------------------------------------------------------------- On Mconley's work about opening popup windows there were some issues with the timing for when this information was available. IIRC by the end of the work on that bug that had been solved, but it's worth double checking if .useRemoteTabs will be correct this early for popup windows too.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3da6dceb4c4d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d2f4788a38 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f61c734e372 I verified that gMultiProcessBrowser is set to the right value when opening popups, so I think we're okay setting it that early.

The session restore tests use chrome:// URLs as test pages in several places. We get more test coverage by loading these remotely. Originally I whitelisted chrome URLs out of an abundance of caution, but I don't think there's any reason a user would load a chrome URL and expect it to be remote. We may want to revisit this for chrome URLs provided by add-ons, but we can worry about that later.

When we first started working on getting mochitests running, we added a listener for common events and passed them on to the parent. Nowadays, tests run in the context of the mochitest add-on, so they get the same event shims as addons do. Therefore there's no reason for this extra code. It's actually harming us by causing us to get two copies of every event.

This patch revises some of the tests to work in e10s. For most tests, CPOWs work fine for touching content. This patch fixes the few places where CPOWs don't work. For example, CPOWs don't currently work correctly when you try to wrap a regexp object. I worked around this by adding a runInContent function that runs code in the content process. It's like a very lightweight frame script.

Comment on attachment 8480840 [details] [diff] [review] remove-weird-event-shim I was curious what the heck this was for, glad to see its not needed anymore.

With these patches and the blocking bugs, the session restore tests pass for me!

Comment on attachment 8480843 [details] [diff] [review] test-changes Review of attachment 8480843 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser_447951.js @@ +38,5 @@ > > function check() { > SyncHandlers.get(tab.linkedBrowser).flush(); > tabState = JSON.parse(ss.getTabState(tab)); > + if (tab.linkedBrowser.currentURI.spec != baseURL + "end") { This is quite different than what we tested before. What you're testing here is checked on line 49 and the tabState itself isn't queried anymore. ::: browser/components/sessionstore/test/browser_500328.js @@ +91,5 @@ > // history entries: > // testURL (state object: null) <-- oldest > // testURL (state object: {obj1:1}) > // testURL?page2 (state object: {obj3:/^a$/}) <-- newest > + let contentTest = function(win) { It's at the top-level, could as well be "function contentTest(win) { ..." ::: browser/components/sessionstore/test/head.js @@ +500,5 @@ > win.close(); > return deferred.promise; > } > > +function runInContent(browser, func, arg, callback = null) { That's a fancy idea, I like it :) We could simplify a couple of broadcasting tests with that. With the recent surge in tests using Task.jsm I think it would be better for runInContent to return a promise though. You could also return a promise and accept a callback if you want.

https://hg.mozilla.org/mozilla-central/rev/3da6dceb4c4d https://hg.mozilla.org/mozilla-central/rev/e3d2f4788a38 https://hg.mozilla.org/mozilla-central/rev/5f61c734e372

Comment on attachment 8480836 [details] [diff] [review] allow-remote-chrome Review of attachment 8480836 [details] [diff] [review]: ----------------------------------------------------------------- Let's do it.

Comment on attachment 8480840 [details] [diff] [review] remove-weird-event-shim I decided to move this patch to bug 1066433.

CCing Mossop in case the chrome: URL change affects his updateRemoteness changes.

This fixes some intermittent orange. A bunch of tests were doing this: SyncHandlers.get(tab.linkedBrowser).flush(); However, flush() actually takes one argument: the message ID of the last message received by the parent. Without this ID, the following can happen: child gets update, sends it via an async message parent calls flush, gets nothing because everything has already been sent async message arrives too late I converted the code to call TabState.flush(browser) instead. I also converted calls to flushAsync to go through TabState too. That way we can eliminate SyncHandlers in head.js, which should make it less likely to commit this error in the future.

Comment on attachment 8492506 [details] [diff] [review] fix-ss-sync-handler Review of attachment 8492506 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Bill McCloskey (:billm) from comment #16) > I converted the code to call TabState.flush(browser) instead. I also > converted calls to flushAsync to go through TabState too. That way we can > eliminate SyncHandlers in head.js, which should make it less likely to > commit this error in the future. Great, thanks for fixing this! The same thing occurred to me a few weeks ago and I wondered why I didn't use TabState.flush() back then... Nice debugging, btw :)

This patch fixes an actual bug where we ignore MozStorageChanged events that occur in iframes. I also changed the test to more easily expose the issue.

This patch addresses a couple related issues: 1. The MozStorageChanged event handler fires in the test code before it fires in content-sessionStore.js. As a consequence, the test can captures the tab state before the content script has observed the change, so it will get the old tab state. I fixed this by adding executeSoon around the test's event handler so that it's guaranteed to run second. 2. When we change a page's style sheet, content-sessionStore.js notices the change using an observer that fires asynchronously. However, the test code assumes that calling enableStyleSheetsForSet will take effect immediately. As a consequence, the test might try to capture the new tab state before the content script has seen the change to the style sheet. I fixed this by using the same observer notification in the test that content-sessionStore.js uses. I also used executeSoon here in case the test's observer runs first.

Comment on attachment 8493516 [details] [diff] [review] ss-storage-fix Review of attachment 8493516 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, stupid mistake. Thanks for catching that!

Comment on attachment 8493522 [details] [diff] [review] ss-test-fixes Review of attachment 8493522 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: browser/components/sessionstore/test/content.js @@ +11,5 @@ > Cu.import("resource:///modules/sessionstore/FrameTree.jsm", this); > let gFrameTree = new FrameTree(this); > > +function executeSoon(callback) { > + Services.tm.mainThread.dispatch(callback, Components.interfaces.nsIThread.DISPATCH_NORMAL); Nit: Ci.nsIThread.DISPATCH_NORMAL @@ +115,5 @@ > > addMessageListener("ss-test:enableStyleSheetsForSet", function (msg) { > + let sheets = content.document.styleSheets; > + let change = false; > + for (var i = 0; i < sheets.length; i++) { Nit: let i = 0 @@ +117,5 @@ > + let sheets = content.document.styleSheets; > + let change = false; > + for (var i = 0; i < sheets.length; i++) { > + change |= sheets[i].disabled != (msg.data.indexOf(sheets[i].title) == -1); > + } Would Array.some() work here? Looks like we could stop iterating when change=true anyway. @@ +122,5 @@ > + if (change) { > + // We don't want to reply until content-sessionStore.js has seen > + // the change. > + let observer = { > + observe: function () { nsIObservers could also just be the function, I'd rather just define: function observer() { // do stuff in here }

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7122479a34 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f86e2014af21 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ffdd2ab998f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8549886f4e54 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/82ad04488ff6 Everything here has landed now. To actually enable these tests, we need to wait for either bug 1049879 or bug 1071409 to be resolved.

https://hg.mozilla.org/mozilla-central/rev/0d7122479a34 https://hg.mozilla.org/mozilla-central/rev/f86e2014af21 https://hg.mozilla.org/mozilla-central/rev/7ffdd2ab998f https://hg.mozilla.org/mozilla-central/rev/8549886f4e54 https://hg.mozilla.org/mozilla-central/rev/82ad04488ff6

Both the IPC bugs are ready to land. Unfortunately bug 999239 regressed a number of these tests. Rather than wait to fix that, I'd like to enable what we can to prevent further regressions.

Filed bug 1075658 for the other tests.

Comment on attachment 8498262 [details] [diff] [review] enable-ss-tests Review of attachment 8498262 [details] [diff] [review]: ----------------------------------------------------------------- SGTM. Let's rather have most of the tests running than none of them.

Oops, meant for this to close. Further work will happen in bug 1075658.