1285941 - [e10s-multi] browser_crashedTabs.js fails with multiple content processes

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P3)

Description

[Gabor Krizsanits (INACTIVE)]

Seems like a bug in the test.

230 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_crashedTabs.js | Second tab should be pending. -

I don't think that for multiple content processes the second tab should really be pending here...

237 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_crashedTabs.js | Second tab should still be pending. -

Or here...


248 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_crashedTabs.js | Second window tab should be crashed - Got , expected true

And here we don't crash the second browser.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

We should make this test deal properly (and test) multiple content processes.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Attachment: Use single content process for this test always.

I think we should keep this test as it is. We're interested in the case when all the crashed tabs are same process, and testing the multiple content process cases should be another test.

Comment 3

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

Comment on attachment 8774412 [details] [diff] [review]
Use single content process for this test always.

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

::: browser/components/sessionstore/test/browser_crashedTabs.js
@@ -15,4 @@
>  });
>  
> -// Allow tabs to restore on demand so we can test pending states
> -Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand");

Sorry to drop in - are you sure we don't need this?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8774412 [details] [diff] [review]
Use single content process for this test always.

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

r=me with mconley's question answered.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #3)
> Comment on attachment 8774412 [details] [diff] [review]
> Use single content process for this test always.
> 
> Review of attachment 8774412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/test/browser_crashedTabs.js
> @@ -15,4 @@
> >  });
> >  
> > -// Allow tabs to restore on demand so we can test pending states
> > -Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand");
> 
> Sorry to drop in - are you sure we don't need this?

You are probably right, let's leave that in. I would love to remove it as I don't see any legitimate reason to have a _user_ pref like that being set when this test starts. But let's not take any risk here.
(some tests do not seem to be careful enough with setting/unsetting this pref)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc42306bbe9c&selectedJob=24520206

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ec85d7b044
browser_crashedTabs.js should always use single content process. r=mrbkap

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8ec85d7b044