Closed Bug 1068412 Opened 5 years ago Closed 5 years ago

resource: substitutions are not always sent to the child process

Categories

(Core :: DOM: Content Processes, defect)

x86_64
Linux
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
mozilla35
Iteration:
35.3
Tracking Status
e10s m3+ ---

People

(Reporter: billm, Assigned: mossop)

Details

Attachments

(1 file, 1 obsolete file)

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.
Component: XPCOM → DOM: Content Processes
tracking-e10s: --- → ?
Assignee: nobody → wmccloskey
(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
Assignee: wmccloskey → dtownsend+bugmail
Iteration: --- → 35.2
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Status: NEW → ASSIGNED
Iteration: 35.2 → 35.3
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #8499688 - Flags: review?(wmccloskey)
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.
Attachment #8499688 - Flags: review?(wmccloskey) → review+
Attached patch patch rev 2Splinter Review
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.
Attachment #8499688 - Attachment is obsolete: true
Attachment #8501178 - Flags: review?(wmccloskey)
Attachment #8501178 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/729529607a3a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.