[e10s] Safe mode isn't sent to content process

RESOLVED FIXED in mozilla41

Status

()

Core
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox41 affected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Right now any content process side check of safe mode will always return true. AFAIK no one has done an audit of the consequence of these safe mode checks being incorrect.

We should fix it or audit and understand the implication of this.
(Assignee)

Comment 1

3 years ago
Requesting e10s triage since safe-mode is barely tested but it is an important troubleshooting tool.
tracking-e10s: --- → ?
(Assignee)

Comment 2

3 years ago
mconley says a good starting point is to see if we can use bug 1162838 to send the data to the content process.
Blocks: 1172491
(Assignee)

Comment 3

3 years ago
How about I propagate the safemode attribute from InitProcessAttributes?

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp?from=ContentChild.cpp&case=true#681

In the parent process safemode is setup pretty early so ideally we can set it up early in the child process. InitProcessAttributes feels like a good place for that.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 4

3 years ago
Created attachment 8623090 [details] [diff] [review]
patch

How about this? This makes it such that part 1 in bug 1171682 works in e10s.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Attachment #8623090 - Flags: review?(wmccloskey)
tracking-e10s: ? → m8+
Comment on attachment 8623090 [details] [diff] [review]
patch

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

::: dom/ipc/ContentChild.cpp
@@ +678,5 @@
>                                    XRE_GetProcessType());
>  #endif
>  
> +    bool isSafeMode = false;
> +    SendGetProcessAttributes(&mID, &mIsForApp, &mIsForBrowser, &isSafeMode);

I think isSafeMode makes more sense as part of SendGetXPCOMProcessAttributes. I don't know why we have both GetXPCOMProcessAttributes and GetProcessAttributes; they seem to do the same thing at basically the same time. But the XPCOM one sends more random stuff like this, so it seems like a better fit.

::: dom/ipc/ContentParent.cpp
@@ +3196,5 @@
>  {
>      *aCpId = mChildID;
>      *aIsForApp = IsForApp();
>      *aIsForBrowser = mIsForBrowser;
> +    *aIsSafeMode = gfxPlatform::InSafeMode();

I'd rather not make this depend on some random gfx code. Can you add GetSafeMode to nsAppRunner.h just like SetSafeMode? That seems a lot more direct.
Attachment #8623090 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8623289 [details] [diff] [review]
patch, r=billm
Attachment #8623090 - Attachment is obsolete: true
Attachment #8623289 - Flags: review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(bgirard)
(Assignee)

Comment 8

3 years ago
Looks like the failure are coming from my qparent:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=25e99bc12482

Landing this now.
Flags: needinfo?(bgirard)
(Assignee)

Comment 14

3 years ago
Backout: 
https://hg.mozilla.org/releases/mozilla-beta/rev/f069ca975718

I didn't notice that gfxPlatform::InSafeMode() was missing from the beta branch.
(Assignee)

Updated

3 years ago
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/ce7146d40eb9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 16

3 years ago
Looks like it didn't match the follow-up with the backout.
status-firefox41: fixed → affected
You need to log in before you can comment on or make changes to this bug.