Closed Bug 1059007 Opened 5 years ago Closed 5 years ago

Get session restore tests working in e10s

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

2.87 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
1.69 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
1.80 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
780 bytes, patch
mconley
: review+
Details | Diff | Splinter Review
5.24 KB, patch
jimm
: review+
Details | Diff | Splinter Review
11.26 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
47.01 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
2.58 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
4.26 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
2.90 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
I have some patches for making this work.
Attached patch cangobackSplinter Review
If a subframe navigates, it can change the value of canGoBack for the top-level frame.
Attachment #8479473 - Flags: review?(felipc)
Attached patch mode-switchSplinter Review
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.
Attachment #8479475 - Flags: review?(felipc)
Attached patch storage-eventSplinter Review
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.
Attachment #8479477 - Flags: review?(ttaubert)
Attachment #8479477 - Flags: review?(ttaubert) → review+
Attachment #8479473 - Flags: review?(felipc) → review+
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.
Attachment #8479475 - Flags: review?(felipc) → review+
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.
Attachment #8480836 - Flags: review?(mconley)
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.
Attachment #8480840 - Flags: review?(jmathies)
Attached patch test-changesSplinter Review
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.
Attachment #8480843 - Flags: review?(ttaubert)
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.
Attachment #8480840 - Flags: review?(jmathies) → review+
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.
Attachment #8480843 - Flags: review?(ttaubert) → review+
Comment on attachment 8480836 [details] [diff] [review]
allow-remote-chrome

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

Let's do it.
Attachment #8480836 - Flags: review?(mconley) → review+
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.
Attachment #8492506 - Flags: review?(ttaubert)
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 :)
Attachment #8492506 - Flags: review?(ttaubert) → review+
OS: Linux → All
Hardware: x86_64 → All
Attached patch ss-storage-fixSplinter Review
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.
Attachment #8493516 - Flags: review?(ttaubert)
Attached patch ss-test-fixesSplinter Review
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.
Attachment #8493522 - Flags: review?(ttaubert)
Comment on attachment 8493516 [details] [diff] [review]
ss-storage-fix

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

Ugh, stupid mistake. Thanks for catching that!
Attachment #8493516 - Flags: review?(ttaubert) → review+
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
}
Attachment #8493522 - Flags: review?(ttaubert) → review+
Attached patch enable-ss-testsSplinter Review
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.
Attachment #8498262 - Flags: review?(ttaubert)
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.
Attachment #8498262 - Flags: review?(ttaubert) → review+
Oops, meant for this to close. Further work will happen in bug 1075658.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.