1068412 - resource: substitutions are not always sent to the child process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

We should be informing the child process of each new substitution here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.cpp#310

Instead, we're only doing it during chrome registration. However, bootstrapped addons frequently register substitutions manually so that they can add/remove them at will.

Comment 1

[Dave Townsend [:mossop]]

(In reply to Bill McCloskey (:billm) from comment #0)
> We should be informing the child process of each new substitution here:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/
> nsResProtocolHandler.cpp#310
> 
> Instead, we're only doing it during chrome registration. However,
> bootstrapped addons frequently register substitutions manually so that they
> can add/remove them at will.

We should probably not even be doing it during chrome registration since that goes through this path anyway

Comment 2

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

Yeah, definitely.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch

This sends a new substitution whenever registered. It has the downside that if we dynamically re-register at runtime a number of resource mappings get set and each one is sent individually. I don't know if it's worth the work to avoid that. What do you think Bill?

Comment 4

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

Comment on attachment 8499688 [details] [diff] [review]
patch

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

Great. Just got a few nits.

::: chrome/nsChromeRegistryChrome.cpp
@@ +459,5 @@
>    nsCOMPtr<nsIProtocolHandler> ph;
>    nsresult rv = io->GetProtocolHandler("resource", getter_AddRefs(ph));
>    NS_ENSURE_SUCCESS_VOID(rv);
>  
> +  // If we were passed a parent than a new child process has been created and

than -> then

@@ +461,5 @@
>    NS_ENSURE_SUCCESS_VOID(rv);
>  
> +  // If we were passed a parent than a new child process has been created and
> +  // has requested all of the chrome so send it the resources too. Otherwise
> +  // resource mappings are sent by the resource protocol handler dynamically

Period at the end.

@@ +462,5 @@
>  
> +  // If we were passed a parent than a new child process has been created and
> +  // has requested all of the chrome so send it the resources too. Otherwise
> +  // resource mappings are sent by the resource protocol handler dynamically
> +  if (aParent) {

Can we include the code that gets the IO service and the resource:// handler in this branch? I guess it makes the function fail less frequently, but that seems fine.

::: netwerk/protocol/res/nsResProtocolHandler.cpp
@@ +309,5 @@
>  // nsResProtocolHandler::nsIResProtocolHandler
>  //----------------------------------------------------------------------------
>  
> +static void
> +SendResourceSubstitution(const nsACString& root, nsIURI *baseURI)

nsIURI* baseURI

@@ +311,5 @@
>  
> +static void
> +SendResourceSubstitution(const nsACString& root, nsIURI *baseURI)
> +{
> +    if (GeckoProcessType_Content == XRE_GetProcessType())

Braces here.

@@ +323,5 @@
> +    }
> +
> +    nsTArray<ContentParent*> parents;
> +    ContentParent::GetAll(parents);
> +    if (!parents.Length())

Braces here.

Comment 5

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/f4953072c20f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-e10s crashes.
https://hg.mozilla.org/integration/fx-team/rev/b1b103778f93

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=831743&repo=fx-team

Comment 7

[Dave Townsend [:mossop]]

https://tbpl.mozilla.org/?tree=Try&rev=758ddba67580

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Only changes in this patch is a fix for the test to make it delete the minidumps from the child process crashes so the test harness doesn't freak out.

Comment 9

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/729529607a3a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/729529607a3a