1332343 - Fix browser_largeAllocation.js to work in e10s-multi

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

See bug 1304140 comment 30

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Fix browser_largeAllocation.js in multi-e10s

Hopefully this should fix the problems mentioned in the above linked comment.

I wasn't able to reproduce the tabcount bug, so that may still be happening. Gabor, is there any chance you could try applying this patch and make sure it passes on your computer?

MozReview-Commit-ID: EdNYf1saOI0

Comment 2

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8828389 [details] [diff] [review]
Fix browser_largeAllocation.js in multi-e10s

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

Sorry but it's still failing on my machine in debug build on OSX:

93 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | Uncaught exception - at chrome://browser/content/tabbrowser.xml:2497 - TypeError: aTab is undefined
Stack trace:
    removeTab@chrome://browser/content/tabbrowser.xml:2497:1
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:346:5
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
94 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | Found an unexpected tab at the end of test run: about:blank - 
Buffered messages finished


OR

90 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | PIDs 2 and 3 should match - Got 5759, expected 5777
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:913
    chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:null:338
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    TaskImpl_run@resource://gre/modules/Task.jsm:319:42
    process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:917:23
    walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
    Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:734:11
    schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:765:7
    completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:702:7
    promise callback*completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:693:7
    TaskImpl_run@resource://gre/modules/Task.jsm:324:15
    promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
    TaskImpl_run@resource://gre/modules/Task.jsm:327:15
    process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:917:23
    walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
    Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:734:11
    schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:765:7
    completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:702:7
    receiveMessage@resource://testing-common/ContentTask.jsm:113:9
91 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | undefined assertion name - Got true, expected false
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:913
    chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:null:339
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    TaskImpl_run@resource://gre/modules/Task.jsm:319:42
    process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:917:23
    walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
    Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:734:11
    schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:765:7
    completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:702:7
    promise callback*completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:693:7
    TaskImpl_run@resource://gre/modules/Task.jsm:324:15
    promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
    TaskImpl_run@resource://gre/modules/Task.jsm:327:15
    process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:917:23
    walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
    Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:734:11
    schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:765:7
    completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:702:7
    receiveMessage@resource://testing-common/ContentTask.jsm:113:9
92 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | There should be 3 tabs - Got 2, expected 3
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:913
    chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:null:343
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    TaskImpl_run@resource://gre/modules/Task.jsm:319:42
    process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:917:23
    walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
    Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:734:11
    schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:765:7
    completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:702:7
    promise callback*completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:693:7
    TaskImpl_run@resource://gre/modules/Task.jsm:324:15
    promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
    TaskImpl_run@resource://gre/modules/Task.jsm:327:15
    process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:917:23
    walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:801:7
    Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:734:11
    schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:765:7
    completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:702:7
    receiveMessage@resource://testing-common/ContentTask.jsm:113:9
93 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | Uncaught exception - at chrome://browser/content/tabbrowser.xml:2497 - TypeError: aTab is undefined
Stack trace:
    removeTab@chrome://browser/content/tabbrowser.xml:2497:1
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:346:5
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    @chrome://mochitests/content/browser/dom/tests/browser/browser_largeAllocation.js:307:9
    Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
94 INFO TEST-UNEXPECTED-FAIL | dom/tests/browser/browser_largeAllocation.js | Found an unexpected tab at the end of test run: about:blank -

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Fix browser_largeAllocation.js in multi-e10s

I built and tested on a macbook, and managed to fix the issue you were running into (as far as I can tell). Let me know if this fixes the problem for you.

MozReview-Commit-ID: EdNYf1saOI0

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8829969 [details] [diff] [review]
Fix browser_largeAllocation.js in multi-e10s

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

Thanks this did the trick! I have to admit that I find the fresh process notation quite confusing, and was thinking about cleaning it up a bit and/or document it some more ( I might not fully understand the reason for this name, so it might make perfectly sense and I'm just missing something). This should be a follow up, I would not want to hold back this patch for sure :) And I can do the work even if you don't have the time for it. Is this flag only for large allocation processes? If so, can we make that explicit in its name somehow? Also, if that's the case shouldn't we just use remoteType here: [1] instead of an extra argument? That would mean we can just extend nsGkAtoms::RemoteType instead of using nsGkAtoms::freshProcess.

[1]: http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/dom/ipc/ContentParent.cpp#648

::: dom/interfaces/base/nsITabChild.idl
@@ +34,5 @@
>                                    [array, size_is(linksCount)] in nsIDroppedLinkItem links);
>  
>    readonly attribute uint64_t tabId;
> +
> +  readonly attribute bool freshProcess;

This should be isFreshProcess or wasFreshProcess, no? Also a detailed comment would be nice what this flag stands for exactly.

::: dom/ipc/TabChild.cpp
@@ +3103,5 @@
>    return IPC_OK();
>  }
>  
> +NS_IMETHODIMP
> +TabChild::GetFreshProcess(bool* aResult)

Here too. I would expect GetFreshProcess to return a process...

::: dom/tests/browser/browser_largeAllocation.js
@@ +289,5 @@
> +    // browsing context was booted out of the fresh process. If we discover that
> +    // this was not a fresh process, we'll need to wait for another process.
> +    // Start listening now.
> +    epc = expectProcessCreated();
> +    if (!(yield getIsFreshProcess(aBrowser))) {

Shouldn't we start listening only in this case?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Comment on attachment 8829969 [details] [diff] [review]
> Fix browser_largeAllocation.js in multi-e10s
> 
> Review of attachment 8829969 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks this did the trick! I have to admit that I find the fresh process
> notation quite confusing, and was thinking about cleaning it up a bit and/or
> document it some more ( I might not fully understand the reason for this
> name, so it might make perfectly sense and I'm just missing something). This
> should be a follow up, I would not want to hold back this patch for sure :)

I also have been wanting to clean this up. The FreshProcess name is a bit of a holdover from previous implementations of this feature, and they should be called things like "isLargeAllocationProcess", and "isLargeAllocationTabChild". I'll file a follow up to this bug to fix this naming.

> And I can do the work even if you don't have the time for it. Is this flag
> only for large allocation processes? If so, can we make that explicit in its
> name somehow? Also, if that's the case shouldn't we just use remoteType
> here: [1] instead of an extra argument? That would mean we can just extend
> nsGkAtoms::RemoteType instead of using nsGkAtoms::freshProcess.

I agree. This large allocation work predates the remoteType concept (which was added by bug 1147911 IIRC), and the patch for large allocation landed later that day. It would be nice to unify the concepts together better.

> [1]:
> http://searchfox.org/mozilla-central/rev/
> 02a56df6474a97cf84d94bbcfaa126979970905d/dom/ipc/ContentParent.cpp#648
> 
> ::: dom/interfaces/base/nsITabChild.idl
> @@ +34,5 @@
> >                                    [array, size_is(linksCount)] in nsIDroppedLinkItem links);
> >  
> >    readonly attribute uint64_t tabId;
> > +
> > +  readonly attribute bool freshProcess;
> 
> This should be isFreshProcess or wasFreshProcess, no? Also a detailed
> comment would be nice what this flag stands for exactly.

SGTM

> 
> ::: dom/ipc/TabChild.cpp
> @@ +3103,5 @@
> >    return IPC_OK();
> >  }
> >  
> > +NS_IMETHODIMP
> > +TabChild::GetFreshProcess(bool* aResult)
> 
> Here too. I would expect GetFreshProcess to return a process...

I agree - this should be changed. I'll change the name to isInFreshProcess, and we can get a better name in the follow-up.

> ::: dom/tests/browser/browser_largeAllocation.js
> @@ +289,5 @@
> > +    // browsing context was booted out of the fresh process. If we discover that
> > +    // this was not a fresh process, we'll need to wait for another process.
> > +    // Start listening now.
> > +    epc = expectProcessCreated();
> > +    if (!(yield getIsFreshProcess(aBrowser))) {
> 
> Shouldn't we start listening only in this case?

In this case, what is happening, is a load is starting, which kicks the load out of the L-A process, and into a normal content process. This load then begins, and the Large-Allocation header is seen again, which kicks the load back into a new L-A process. 

As getIsFreshProcess is async, I was worried that the process created event could be fired while we were waiting for the response to be sent, so I hooked up the listener ASAP, and canceled it if we decided we didn't need it. In my tests this didn't cause a problem, but I didn't want to create an avoidable intermittent on infra.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5122878d7791
Fix browser_largeAllocation.js in multi-e10s, r=smaug

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5122878d7791

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8829969 [details] [diff] [review]
Fix browser_largeAllocation.js in multi-e10s

Approval Request Comment

This is part 2 of a 6-bug/7-patch uplift request

[Feature/Bug causing the regression]: We would like to ship the Large-Allocation header in 52/53, and these patches make some final touch-ups which will be required before we can ship.
[User impact if declined]: Increased crash rates of wasm games on 32-bit windows
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Most of these fixups just landed in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: 
bug 1331083
bug 1332343
bug 1334210
bug 1333936
bug 1334586
bug 1331087
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: This exposes and enables an opt-in low risk web API which is only supported by Firefox to help avoid OOM problems on 32-bit windows.
[String changes made/needed]: A string was added to dom.properties in bug 1331087

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8829969 [details] [diff] [review]
Fix browser_largeAllocation.js in multi-e10s

Let's try this in aurora, part of the large-allocation header fix for OOM crashes along with bug 1331083.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce5e1c705787