Closed
Bug 1068412
Opened 10 years ago
Closed 10 years ago
resource: substitutions are not always sent to the child process
Categories
(Core :: DOM: Content Processes, defect)
Tracking
()
Tracking | Status | |
---|---|---|
e10s | m3+ | --- |
People
(Reporter: billm, Assigned: mossop)
Details
Attachments
(1 file, 1 obsolete file)
17.12 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Component: XPCOM → DOM: Content Processes
Updated•10 years ago
|
tracking-e10s:
--- → ?
Updated•10 years ago
|
Assignee: nobody → wmccloskey
Assignee | ||
Comment 1•10 years ago
|
||
(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
Reporter | ||
Comment 2•10 years ago
|
||
Yeah, definitely.
Assignee | ||
Updated•10 years ago
|
Assignee: wmccloskey → dtownsend+bugmail
Iteration: --- → 35.2
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
Iteration: 35.2 → 35.3
Assignee | ||
Comment 3•10 years ago
|
||
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)
Reporter | ||
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f4953072c20f
Comment 6•10 years ago
|
||
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
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=758ddba67580
Assignee | ||
Comment 8•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Attachment #8501178 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/729529607a3a
https://hg.mozilla.org/mozilla-central/rev/729529607a3a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•