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

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Session Restore
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gabor Krizsanits (INACTIVE), Assigned: Gabor Krizsanits (INACTIVE))

Tracking

unspecified
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [e10s-multi:M1])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1207306
Whiteboard: [e10s-multi:M?]
tracking-e10s: --- → ?
(Assignee)

Updated

2 years ago
tracking-e10s: ? → ---

Updated

2 years ago
Blocks: 984139
tracking-e10s: --- → +
(Assignee)

Updated

2 years ago
No longer blocks: 984139
tracking-e10s: + → ---
Priority: -- → P3
We should make this test deal properly (and test) multiple content processes.
Whiteboard: [e10s-multi:M?] → [e10s-multi:M1]
(Assignee)

Updated

2 years ago
Assignee: nobody → gkrizsanits
(Assignee)

Comment 2

2 years ago
Created attachment 8774412 [details] [diff] [review]
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.
Attachment #8774412 - Flags: review?(mrbkap)
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 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.
Attachment #8774412 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

2 years ago
(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

2 years ago
Pushed by gkrizsanits@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ec85d7b044
browser_crashedTabs.js should always use single content process. r=mrbkap

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8ec85d7b044
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.