1059007 - Get session restore tests working in e10s

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I have some patches for making this work.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cangoback

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

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: mode-switch

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: storage-event

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 4

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: allow-remote-chrome

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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove-weird-event-shim

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.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test-changes

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 9

[Jim Mathies [:jimm]]

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.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

[Carsten Book [:Tomcat]]

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 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8480836 [details] [diff] [review]
allow-remote-chrome

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

Let's do it.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8480840 [details] [diff] [review]
remove-weird-event-shim

I decided to move this patch to bug 1066433.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-ss-sync-handler

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 17

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: ss-storage-fix

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.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: ss-test-fixes

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 20

[Tim Taubert [:ttaubert] (inactive)]

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 21

[Tim Taubert [:ttaubert] (inactive)]

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
}

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: enable-ss-tests

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.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Filed bug 1075658 for the other tests.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/741b66e8ecd4

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/741b66e8ecd4

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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