Replace duplicated code: XRE_GetProcessType() == GeckoProcessType_Default/GeckoProcessType_Content by XRE_IsParentProcess() / XRE_IsContentProcess()

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: _AtilA_, Assigned: _AtilA_)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

We found that this comparison: XRE_GetProcessType() == GeckoProcessType_Default [1] is duplicated all over the code. There's already a function defined in XRE toolkit called: XRE_IsParentProcess(), that looks a good candidate to use instead of all these duplicates. For the GeckoProcessType_Content comparison, we could make another function called: XRE_IsContentProcess() in the same toolkit and replace all the comparisons as well.

[1] https://dxr.mozilla.org/mozilla-central/search?q=XRE_GetProcessType%28%29&redirect=true
Assignee: nobody → atilag
See Also: → 1087161
Blocks: 1087161
This is a big patch, it modifies a lot of components all over the code, so before setting this patch to be reviewed I would like to launch a try job to be sure that it doesn't break anything, but looks like the try-server is CLOSED right now (bug 1172750).

Things to consider:
* Added new function to toolkit/xre/nsAppRunner.cpp: XRE_IsContentProcess() that replaced a lot of comparisons like:  XRE_GetProcessType() == GeckoProcessType_Content
* Removed IsMainProcess() from dom/asmjscache/AsmJSCache.cpp and dom/bluetooth/bluetooth1/BluetoothService.cpp because XRE_IsParentProcess() does exactly the same thing.
I needed to add XRE_IsParentProcess and XRE_IsContentProcess to libxpcomrt, because otherwise it doesn't compile in Linux and some other Android platforms.
Attachment #8617372 - Attachment is obsolete: true
Attachment #8620562 - Flags: review?(mh+mozilla)
Attachment #8620562 - Flags: review?(mh+mozilla) → review?(nfroyd)
Comment on attachment 8620562 [details] [diff] [review]
0001-Bug-1171931-Duplicated-code-replaced-by-XRE_IsParent.patch

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

r=me with the changes below.  The ContentParent.cpp change was just me thinking out loud; it should probably be a separate bug.

I recall Benjamin disliking something about a change like this, but I can't find it in my bugmail.  Benjamin, do you have any recollection of that?

::: dom/datastore/DataStoreService.cpp
@@ +133,1 @@
>    return isMainProcess;

Seems you like you might as well replace uses of this function to call XRE_IsParentProcess() directly.

::: dom/filesystem/FileSystemUtils.cpp
@@ +69,4 @@
>  bool
>  FileSystemUtils::IsParentProcess()
>  {
> +  return XRE_IsParentProcess();

And maybe this function, too.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +252,4 @@
>    }
>  
>    if (!gDBManager) {
> +    sIsMainProcess = XRE_IsParentProcess();

Also possibly worthwhile to replace uses of this variable.

::: dom/ipc/ContentParent.cpp
@@ +1144,4 @@
>      }
>  
>      ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
> +    bool isInContentProcess = !XRE_IsParentProcess();

I guess we should really be using XRE_IsContentProcess here?  Maybe that would cause other problems...

::: dom/quota/QuotaManager.cpp
@@ +760,4 @@
>  bool
>  IsMainProcess()
>  {
> +  return XRE_IsParentProcess();

The uses of this function could be replaced, too.

::: ipc/glue/BackgroundImpl.cpp
@@ +77,1 @@
>    return isMainProcess;

Again, another function that could be replaced.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +26,4 @@
>    property_set("ctl.start", "mdnsd");
>  }
>  
> +inline void

This looks like a spurious change.
Attachment #8620562 - Flags: review?(nfroyd)
Attachment #8620562 - Flags: review+
Attachment #8620562 - Flags: feedback?(benjamin)
Thanks Nathan,
I changed almost all of your indications.
I'll wait for Benjamin comments to move forward.
 
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Comment on attachment 8620562 [details] [diff] [review]
> 0001-Bug-1171931-Duplicated-code-replaced-by-XRE_IsParent.patch
> 
> Review of attachment 8620562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the changes below.  The ContentParent.cpp change was just me
> thinking out loud; it should probably be a separate bug.
> 
> I recall Benjamin disliking something about a change like this, but I can't
> find it in my bugmail.  Benjamin, do you have any recollection of that?
> 
> ::: dom/datastore/DataStoreService.cpp
> @@ +133,1 @@
> >    return isMainProcess;
> 
> Seems you like you might as well replace uses of this function to call
> XRE_IsParentProcess() directly.
> 
> ::: dom/filesystem/FileSystemUtils.cpp
> @@ +69,4 @@
> >  bool
> >  FileSystemUtils::IsParentProcess()
> >  {
> > +  return XRE_IsParentProcess();
> 
> And maybe this function, too.
> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +252,4 @@
> >    }
> >  
> >    if (!gDBManager) {
> > +    sIsMainProcess = XRE_IsParentProcess();
> 
> Also possibly worthwhile to replace uses of this variable.
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +1144,4 @@
> >      }
> >  
> >      ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
> > +    bool isInContentProcess = !XRE_IsParentProcess();
> 
> I guess we should really be using XRE_IsContentProcess here?  Maybe that
> would cause other problems...
> 
> ::: dom/quota/QuotaManager.cpp
> @@ +760,4 @@
> >  bool
> >  IsMainProcess()
> >  {
> > +  return XRE_IsParentProcess();
> 
> The uses of this function could be replaced, too.
> 
> ::: ipc/glue/BackgroundImpl.cpp
> @@ +77,1 @@
> >    return isMainProcess;
> 
> Again, another function that could be replaced.
> 
> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
> @@ +26,4 @@
> >    property_set("ctl.start", "mdnsd");
> >  }
> >  
> > +inline void
> 
> This looks like a spurious change.

I needed to add this inline here because this is an unused function and it's causing me a compile error with gcc-4.9. The author of this code (Gary Chen [:xeonchen]) opened a new bug (bug 1172383) to address this issue, meanwhile I added the inline to please the compiler. It should disappear once the function will be finally used, I'll write a comment in the bug to remind this change.
My concern about potential IsParent/IsContent functions was that they would be used in a context where we could have another process type: GMP or plugin, for example. So it would not be ok to use IsParent/IsContent in the IPC layer, for example.

So I believe that if we have IsParent/IsContent functions, they should only be used in contexts where we only expect to in a parent or content, and so it should assert that the type is either content or chrome and not the other types.
Comment on attachment 8620562 [details] [diff] [review]
0001-Bug-1171931-Duplicated-code-replaced-by-XRE_IsParent.patch

The tree changes look fine in this context: I just want more assertions.
Attachment #8620562 - Flags: feedback?(benjamin) → feedback-
Some deeper refactoring on DataStoreService.cpp, merging MOZ_ASSERTS() and getting rid of unnecessary functions.
Attachment #8620562 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21d9e72105c7
(red builds are caused because of some try-server issues...)
Keywords: checkin-needed
this failed to apply:

applying 0001-Bug-1171931-Refactoring-duplicatd-code-using-XRE_IsP.patch
patching file dom/ipc/ProcessPriorityManager.cpp
Hunk #2 succeeded at 429 with fuzz 2 (offset 3 lines).
Hunk #3 succeeded at 661 with fuzz 1 (offset 4 lines).
patching file dom/plugins/ipc/PluginModuleParent.cpp
Hunk #1 succeeded at 354 with fuzz 2 (offset 6 lines).
patching file dom/workers/ServiceWorkerManager.cpp
Hunk #1 FAILED at 365
Hunk #2 FAILED at 4129
2 out of 2 hunks FAILED -- saving rejects to file dom/workers/ServiceWorkerManager.cpp.rej
patching file xpcom/build/XPCOMInit.cpp
Hunk #3 FAILED at 953
1 out of 4 hunks FAILED -- saving rejects to file xpcom/build/XPCOMInit.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 0001-Bug-1171931-Refactoring-duplicatd-code-using-XRE_IsP.patch
Flags: needinfo?(atilag)
Keywords: checkin-needed
Merge conflict fixed
Attachment #8625270 - Attachment is obsolete: true
Flags: needinfo?(atilag)
Keywords: checkin-needed
This still doesn't apply on inbound tip.
Keywords: checkin-needed
Ok, this last patch merged with b2g-inbound. It's kind of large (but simple) patch, so I hope there's no more merging issues.
Attachment #8625717 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out for breaking debug emulator-kk & emulator-l builds.
https://treeherder.mozilla.org/logviewer.html#?job_id=11106516&repo=mozilla-inbound
Merged with m-i and verified that it runs... (once again, despite of timeouts in some builds)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7006519fddc
Keywords: checkin-needed
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2225502&repo=b2g-inbound
Flags: needinfo?(atilag)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7262c6d249
(there's one more commit that shouldn't be there, but for the purpose of the job, it's ok)
Attachment #8625875 - Attachment is obsolete: true
Flags: needinfo?(atilag)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ac19d3bf7bf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
A dev.platform post letting people know the convention has changed might be good.
You need to log in before you can comment on or make changes to this bug.