551181 - Tell the chrome process when we visit a page

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Frederic Plourde (:fred23)]

Remote AddVisit() calls from content to chrome process so that it can add a visit to the history database properly.

Comment 1

[Frederic Plourde (:fred23)]

Well, in fact, the entry point to adding a new Visit from content is really through nsDocShell::AddToGlobalhistory(...), which is consumed by many nsDocShell foos like : ::OnNewURI(), ::OnRedirectStateChange() and ::AddState().

Comment 2

[Frederic Plourde (:fred23)]

Attachment: patch v.1

This is "part2" of the remote link-visited issue in e10s.
By applying this patch *on top* of patch for bug 516728 (part1), link coloring in e10s becomes operational.

I still dislike all the ""// reconstruct our IPDL-passed URIs"" parts, but there's a bug already filed (Bug 541174) for passing nsIURI across process boundary in a more optimized way.

Comment 3

[Boris Zbarsky [:bzbarsky]]

A few quick notes:

1)  No need to assign "" to aReferrerSpec.  It's already empty.
2)  Please don't use the aFoo naming convention for things that aren't arguments.
3)  In MOZ_IPC builds in the content process, we'll notify the content-side
    mGlobalHistory too.  Is that what we want?
4)  Why are we overriding the failing rv in RecvIsVisited?
5)  If we want to check whether aReferrerSpec is empty, just use IsEmpty() on the
    string instead of EqualsLiteral(""), please.

Comment 4

[Frederic Plourde (:fred23)]

Attachment: patch v.2

Patch corrected per Boris's comments.

(In reply to comment #3)
> 3)  In MOZ_IPC builds in the content process, we'll notify the content-side
>     mGlobalHistory too.  Is that what we want?

I'm not sure I understand what you mean here. really, the content-side mGlobalHistory is 0x00000000 (not accessible from content)... Tell me more, it's possible I've overlooked something here. thanks.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  Your code just makes it so that visiting a link in the chrome process doesn't add it to history.  Is that desired?  I guess we do that right now for type="chrome" docshells, but I believe the current e10s setup is that you can have type="content" docshells in the chrome process...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Also, as currently written ifdef MOZ_IPC and if we're not in the content process then visited never gets set and we branch on a random value.

Comment 7

[Frederic Plourde (:fred23)]

Attachment: patch v.3

(In reply to comment #6)
> Also, as currently written ifdef MOZ_IPC and if we're not in the content
> process then visited never gets set and we branch on a random value.

Yeah, you're right! I just didn't think at first that the chrome process could use this codepath. Here's an updated patch. This version induces a tiny code redundancy between the MOZ_IPC and non-MOZ_IPC cases, but I prefer that to putting that code in a new function.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> This version induces a tiny code redundancy 

Why, though?  This should work fine:

#ifdef MOZ_IPC
  if (XRE_GetProcessType() == GeckoProcessType_Content) {
  }
  else
#endif /* MOZ_IPC */
  if (mGlobalHistory) {
    // Code for chrome-process stuff and non-moz-ipc stuff here.
  }

and wouldn't require that initial ifdefed !mGlobalHistory check if you also condition the notification below that on mGlobalHistory.

Comment 9

[Boris Zbarsky [:bzbarsky]]

One other nit, while I'm here.  Sentence-like comments need capitalization and punctuation (and use of ';', not ',', as appropriate).

Comment 10

[Frederic Plourde (:fred23)]

(In reply to comment #8)
> Why, though?  This should work fine:
> 
> #ifdef MOZ_IPC
>   if (XRE_GetProcessType() == GeckoProcessType_Content) {
>   }
>   else
> #endif /* MOZ_IPC */
>   if (mGlobalHistory) {
>     // Code for chrome-process stuff and non-moz-ipc stuff here.
>   }

Well, I had considered this option, but I personally don't like it because having "if()...else" blocks cross #ifdef boundaries break indentation levels and make the code harder to read, but as I see it's allright with you, I can update the patch accordingly, since it has indeed some interesting advantages, like removing code duplication.

> and wouldn't require that initial ifdefed !mGlobalHistory check if you also
> condition the notification below that on mGlobalHistory.

true.

Comment 11

[Frederic Plourde (:fred23)]

Attachment: patch v.4

Here's patch v.4, per bz's comments. thanks.

Comment 12

[Frederic Plourde (:fred23)]

Attachment: patch v.5

Fixed bitrot.

Comment 13

[Frederic Plourde (:fred23)]

Comment on attachment 434279 [details] [diff] [review]
patch v.5

Removing the review request for now, as this bug is going to be affected by Stover's work (bug 556400). When his work posts/lands, I'll retouch this bug and ask for a review.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.6

Comment 15

[Josh Matthews [:jdm]]

>+NS_ASSERTION(!(aReferrer && aRedirectFrom), "Both can not be non-null");

That message is needlessly complicated to parse.

Comment 16

[Doug Turner (:dougt)]

the patch is obsolete for ^ and other reasons. ;-)

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 446030 [details] [diff] [review]
patch v.6

lets review this and ignore that assertion.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 446030 [details] [diff] [review]
patch v.6

per in-person discussion with dougt

Comment 19

[Doug Turner (:dougt)]

Attachment: patch

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
>--- a/dom/ipc/ContentProcessParent.cpp
>+++ b/dom/ipc/ContentProcessParent.cpp
>@@ -41,17 +41,17 @@
> 
> #include "TabParent.h"
> #include "mozilla/ipc/TestShellParent.h"
> #include "mozilla/net/NeckoParent.h"
> #include "nsIPrefBranch.h"
> #include "nsIPrefBranch2.h"
> #include "nsIPrefLocalizedString.h"
> #include "nsIObserverService.h"
>-
>+#include "nsContentUtils.h"
> #include "nsAutoPtr.h"
> #include "nsCOMPtr.h"
> #include "nsServiceManagerUtils.h"
> #include "nsThreadUtils.h"
> #include "nsChromeRegistryChrome.h"
> 
> using namespace mozilla::ipc;
> using namespace mozilla::net;
>@@ -373,16 +373,29 @@ ContentProcessParent::RequestRunToComple
>         printf("Running to completion...\n");
> #endif
>         mRunToCompletionDepth = 1;
>         mShouldCallUnblockChild = true;
>     }
>     return !!mRunToCompletionDepth;
> }
> 
>+
>+bool
>+ContentProcessParent::RecvVisitURI(const IPC::URI& uri,
>+                                   const IPC::URI& referrer,
>+                                   const PRUint32& flags)
>+{
>+    nsCOMPtr<nsIURI> ourURI = uri;
>+    nsCOMPtr<nsIURI> ourReferrer = referrer;
>+    IHistory *history = nsContentUtils::GetHistory(); 
>+    history->VisitURI(ourURI, referrer, flags);

I would have expected a NULL-check here but I don't know that the
GetHistory() API contract is (looks to be in a pending patch).

>diff --git a/dom/ipc/PContentProcess.ipdl b/dom/ipc/PContentProcess.ipdl
>--- a/dom/ipc/PContentProcess.ipdl
>+++ b/dom/ipc/PContentProcess.ipdl
>@@ -37,17 +37,19 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> include protocol PIFrameEmbedding;
> include protocol PTestShell;
> include protocol PNecko;
> 
> include "mozilla/TabTypes.h";
> include "mozilla/chrome/RegistryMessageUtils.h";
>+include "mozilla/net/NeckoMessageUtils.h";
> 
>+using IPC::URI;
> using ChromePackage;
> using ResourceMapping;
> using OverrideMapping;
> 
> namespace mozilla {
> namespace dom {
> 
> rpc protocol PContentProcess
>@@ -66,16 +68,18 @@ child:
> 
>     async SetOffline(PRBool offline);
> 
>     NotifyRemotePrefObserver(nsCString aDomain);
> 
> parent:
>     PNecko();
> 
>+    async VisitURI(URI uri, URI referrer, PRUint32 flags);
>+

Is it worth validating flags sent by the content process?  Can it
cause mischief by sending bad flags?  (Looks like the patch that adds
the flags hasn't landed yet so I don't know what they are.)

>diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp
>--- a/toolkit/components/places/src/History.cpp
>+++ b/toolkit/components/places/src/History.cpp
>@@ -32,16 +32,21 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#ifdef MOZ_IPC
>+#include "mozilla/dom/ContentProcessChild.h"
>+#include "nsXULAppAPI.h"
>+#endif
>+
> #include "History.h"
> #include "nsNavHistory.h"
> #include "nsNavBookmarks.h"
> #include "Helpers.h"
> 
> #include "mozilla/storage.h"
> #include "mozilla/dom/Link.h"
> #include "nsDocShellCID.h"
>@@ -789,16 +794,26 @@ History::StartNextVisitURI()
> 
> NS_IMETHODIMP
> History::VisitURI(nsIURI* aURI,
>                   nsIURI* aLastVisitedURI,
>                   PRUint32 aFlags)
> {
>   NS_PRECONDITION(aURI, "URI should not be NULL.");
> 
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    mozilla::dom::ContentProcessChild * cpc = 
>+      mozilla::dom::ContentProcessChild::GetSingleton();

|using mozilla::dom::ContentProcessChild| please.

Comment 21

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh on the places changes

Comment 22

[Doug Turner (:dougt)]

http://hg.mozilla.org/projects/electrolysis/rev/71114e4522c7

Comment 23

[Doug Turner (:dougt)]

this was backed out a few days ago because the required places patches were backed out.

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v1.1

just the patch Doug pushed to tryserver, that applies.

Comment 25

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/da160d0117e7