Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Description • 9 years ago

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

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 1 • 9 years ago

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.

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 2 • 9 years ago

I needed to add XRE_IsParentProcess and XRE_IsContentProcess to libxpcomrt, because otherwise it doesn't compile in Linux and some other Android platforms.

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 3 • 9 years ago

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

	Nathan Froyd [:froydnj]
	Comment 4 • 9 years ago

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.

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 5 • 9 years ago

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.

	Benjamin Smedberg
	Comment 6 • 9 years ago

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.

	Benjamin Smedberg
	Comment 7 • 9 years ago

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.

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 8 • 9 years ago

Some deeper refactoring on DataStoreService.cpp, merging MOZ_ASSERTS() and getting rid of unnecessary functions.

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 9 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21d9e72105c7
(red builds are caused because of some try-server issues...)

	Carsten Book [:Tomcat]
	Comment 10 • 9 years ago

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

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 11 • 9 years ago

Merge conflict fixed

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 9 years ago

This still doesn't apply on inbound tip.

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 13 • 9 years ago

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.

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4e4083639e

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago

Backed out for breaking debug emulator-kk & emulator-l builds.
https://treeherder.mozilla.org/logviewer.html#?job_id=11106516&repo=mozilla-inbound

	Pulsebot
	Comment 16 • 9 years ago

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

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 17 • 9 years ago

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

	Pulsebot
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/f41a2121425f

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2225502&repo=b2g-inbound

	Pulsebot
	Comment 20 • 9 years ago

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/79800010be78

	Juan Gomez [:_AtilA_] (CET/CEST) Assignee
	Comment 21 • 9 years ago

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)

	Pulsebot
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac19d3bf7bf

	Phil Ringnalda (:philor)
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0ac19d3bf7bf

	Jim Mathies [:jimm]
	Comment 24 • 9 years ago

A dev.platform post letting people know the convention has changed might be good.