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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Benoit Girard (:BenWa)]

Requesting e10s triage since safe-mode is barely tested but it is an important troubleshooting tool.

Comment 2

[Benoit Girard (:BenWa)]

mconley says a good starting point is to see if we can use bug 1162838 to send the data to the content process.

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

How about this? This makes it such that part 1 in bug 1171682 works in e10s.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch, r=billm

Comment 7

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7864171d7d12

Comment 8

[Benoit Girard (:BenWa)]

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

Landing this now.

Comment 9

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8601df335c1

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7146d40eb9

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4060e527bd2

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/545afafa3df4

Comment 13

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2a19a8f46a2

Comment 14

[Benoit Girard (:BenWa)]

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

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Benoit Girard (:BenWa)]

Looks like it didn't match the follow-up with the backout.