Closed Bug 516728 Opened 15 years ago Closed 14 years ago

Remote link-visited information

Categories

(Core :: IPC, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: benjamin, Assigned: dougt)

References

Details

Attachments

(1 file, 13 obsolete files)

7.51 KB, patch
Details | Diff | Splinter Review
Remote the link-visited information from the chrome process to the content process. This mostly depends on an asynchronous link-visited mechanism in layout, which sdwilsh is doing in bug 461199.
Probably best if somebody else picks this up if you want it anytime in December.  Bug 461199 is taking a lot longer than originally expected.  Anybody who wants to take this just needs to apply attachment 414533 [details] [diff] [review] which adds the API that needs to be remoted.
Assignee: sdwilsh → nobody
Assignee: nobody → bugzillaFred
This is a diagram of how mozilla::dom::Link and mozilla::IHistory interact.  It also shows some more details on the state of things inside of IHistory.  The red, lower box shows what happens inside of IHistory, and the green, upper box shows what Link will do.

Some invariants to note:
1) Link must call UnregisterVisitedCallback if it has not had SetLinkState called on it (after it has called RegisterVisitedState).  IHistory can assume the Link is still alive until it is told to unregister or it calls SetLinkState on the Link.
2) The URI must be the same in RegisterVisitedState and UnregisterVisitedState.

Those are the only two invariants I think you'll have to worry about in this bug.
Thanks for the diagram, Shawn!
I understand the problem and already designed my coding plan.
I'll ping you if any question arises.
Attached image preliminary flowchart (obsolete) —
Shawn: this flowchart illustrates my intended design for this bug. Do you mind taking a look at it... I'd have some questions for you afterwards, mainly about "NotifyVisited(...)". thanks!
Attached image preliminary flowchart v.2 (obsolete) —
Shawn: please, use this diagram instead...  thanks.
Attachment #423515 - Attachment is obsolete: true
Attached patch W.I.P patch (obsolete) — Splinter Review
This is a w.i.p patch for discussion purposes only. 

Indeed, last week, cjones and I discussed a more complicated design than this one (diagram : https://bug516728.bugzilla.mozilla.org/attachment.cgi?id=423516), but I'd still like that we consider this w.i.p patch more in-depth. I think it's a very elegant and simple solution, as it only modifies *History* instead of hacking the different history consumers. It uses PContentProcessParent/Child to pass requests around (this design has also been proposed and accepted for bug 506269 for remoting preferences from child to parent). One of the advantages is that we *don't* have to keep a separate list of visitedness listeners on the child side... we just let History handle that. On the other hand, however, we don't have a "per-page" list of our listeners so we can't drive un-registration during page-Unload in a *single* IPC call... so we get more IPDL traffic (Every link that wants to unregister must call UnregisterVisitedCallback through IPDL). This still seems acceptable to me though, but please, correct me if I'm wrong. What do you think ?
I don't get the protocol here. On the child side, global history for a particular URL can be in one of three states: unknown/visited/unvisited.

When a content-process client first asks for history information, if we already know the visited state we can just tell them and avoid IPC altogether. If we don't know the state, we need to fire off a message "async StartVisitedQuery(nsCString aURISpec)". So far so good.

How is the parent supposed to respond to that request? With "async NotifyVisited(nsCString aURISpec)"? But that doesn't have a boolean to let you know whether the URL is visited or unvisited? Or do you just assume that when you fire off StartVisitedQuery that the local view goes from "unknown" to "unvisited" and the chrome process will only let you know if the URL is in fact visited?

Also, you'll need to make sure the specs are in a known charset, presumably UTF8.
(In reply to comment #7)
> How is the parent supposed to respond to that request? With "async
> NotifyVisited(nsCString aURISpec)"? But that doesn't have a boolean to let you
> know whether the URL is visited or unvisited? Or do you just assume that when
> you fire off StartVisitedQuery that the local view goes from "unknown" to
> "unvisited" and the chrome process will only let you know if the URL is in fact
> visited?
Even in the single process case, we assume unvisited immediately (this is handled in mozilla::dom::Link) per the API in mozilla::IHistory.  IHistory will notify if it is visited until you unregister.
Yeah, I'm not talking about IHistory, I'm talking about the IPDL protocol assumptions.
From the IPDL protocol p.o.v, the parent indeed replies to the initial request with an async NotifyVisited(aURI).

we then just assume the "visited" state on the child side. Notice that the SendNotifyVisited IPDL message is sent from QueryVisited::HandleCompletion, which, if I understand correctly, gets called back only when the link gets visited.
(In reply to comment #9)
> Yeah, I'm not talking about IHistory, I'm talking about the IPDL protocol
> assumptions.

AFAICT this protocol works the same way as in-process IHistory.  For a "page's" lifetime, every link starts out unvisited.  It can only transition from unvisited->visited, and when that happens, the "page" stops caring about the link's visitedness state.  Frederic's protocol works the same way; NotifyVisited() means "transition from unvisited->visited, and stop caring about this link."  You're right that this protocol is insufficient if async IHistory supported other states or other transitions, but it doesn't.  And that's sdwilsh department.

(In reply to comment #6)
> On the other hand, however, we don't
> have a "per-page" list of our listeners so we can't drive un-registration
> during page-Unload in a *single* IPC call... so we get more IPDL traffic (Every
> link that wants to unregister must call UnregisterVisitedCallback through
> IPDL). This still seems acceptable to me though, but please, correct me if I'm
> wrong. What do you think ?

I don't see any UnregisterVisitedCallback() code in that patch.

Overall I think this approach is fine.  Routing all IPC through IHistory in the child allows numerous IPC-traffic optimizations, so I wouldn't worry about IPC overhead yet.
(In reply to comment #11)

> I don't see any UnregisterVisitedCallback() code in that patch.

Yeah, but from how I designed it, we don't need any modification in UnregisterVisiteCallback because it's going to be run from the child (as the most part of RegisterVisitedCallback). See, the "mObservers" hashtable that gets manipulated from ::RegisterVisitedCallback and ::UnregisterVisitedCallback, that will stay in content process and the only thing I remote to the parent process are the requests for DB checks. This let me only send the requested URI through the IPC pipe (instead of URI *and* Link). When the notification comes back from the parent (NotifyVisited), the visited URI is used as a key in the child-living "mObservers" hashTable to retrieve and notify the associated Link. How does that sound to you ?  

If I had to remote the whole thing, I'm not sure how I would have handled Link objects being passed around.
Blocks: 546938
Attached patch patch v.1 (obsolete) — Splinter Review
Here it is. patch for remoting link-visited info from child to parent in the IPC case. It's been tested successfully with xpcshell wrapper test (bug 546938) so everything looks fine internally and the IHistory invariants appear to be maintained.

Important : note that this patch sends out VisitedRequests in a "sync" manner to be able to retreive an error rv and recover accordingly. For notifications sent to the child, though, the IPC messsagery in "async". I'd like your input/opinion on that particular matter. thanks.
Attachment #424604 - Attachment is obsolete: true
Attachment #431355 - Flags: review?(sdwilsh)
Attached patch patch v.2 (obsolete) — Splinter Review
corrected a tiny glitch in v.1, use this ont instead.

Here it is. patch for remoting link-visited info from child to parent in the
IPC case. It's been tested successfully with xpcshell wrapper test (bug 546938)
so everything looks fine internally and the IHistory invariants appear to be
maintained.

Important : note that this patch sends out VisitedRequests in a "sync" manner
to be able to retreive an error rv and recover accordingly. For notifications
sent to the child, though, the IPC messsagery in "async". I'd like your
input/opinion on that particular matter. thanks.
Attachment #431355 - Attachment is obsolete: true
Attachment #431434 - Flags: review?(sdwilsh)
Attachment #431355 - Flags: review?(sdwilsh)
That NewURI traffic worries me.  :(  Maybe we should just try to make URI-creation a lot faster....
Yeah, having a serializer that can take an nsIURI* and directly create a new URI from it would be nice. Right now in some necko patches we're serializing nsIURI by (spec, charset), and going through NS_NewURI on the other side. If we had a way to know which implementation to construct (i.e. nsStandardURL), even if this fast path existed for just that special case, the serializer could shortcut a lot of work...
(In reply to comment #15)
> That NewURI traffic worries me.  :(  Maybe we should just try to make
> URI-creation a lot faster....

Is there already a bug for that (nsIURI issue) ?  I suggest we file one.
(In reply to comment #17)
> Is there already a bug for that (nsIURI issue) ?  I suggest we file one.

AFAIK, no.
Bug 541174
Comment on attachment 431434 [details] [diff] [review]
patch v.2

Trying to get you feedback faster; dwitte offered to do a first pass review on this.  I'll still need to review it, but he can take a look and offer comments.
Attachment #431434 - Flags: review?(sdwilsh) → review?(dwitte)
yeah, sure!
Comment on attachment 431434 [details] [diff] [review]
patch v.2

I found one problem with this patch.
When IPC prefs (tabs, plugins) disabled and we starting normal fennec with e10s xulrunner, it create some unhandled tab process.

Sounds like this history patch is calling GetSingleton (which is creating remote process), and don't check for prefs.
Good catch ! I'll reproduce and update the patch. thanks
Comment on attachment 431434 [details] [diff] [review]
patch v.2

Canceling review pending updated patch.
Attachment #431434 - Flags: review?(dwitte)
Attached patch patch v.3 (obsolete) — Splinter Review
Here's an updated patch with the following modifs. :
  - fixed bitrot.
  - Defined ContentProcessParent as an History observer (for NotifyVisited events) and thus,  
  - Stopped considering contentProcessParent as a Singleton. (as ContentProcessParent is not going to be a Singleton very long)

Additionnally, it solves Oleg's problem (see comment 22)

Dan : You said you could take a look at this one. I appreciate, thanks ! And as soon as you're finished, I'll r? sdwilsh. I'd like to push that before end of Phase II, thanks.
Attachment #431434 - Attachment is obsolete: true
Attachment #435189 - Flags: feedback?(dwitte)
Comment on attachment 435189 [details] [diff] [review]
patch v.3

Not going to comment on the architecture here, particularly the History changes... just the e10s bits.

>diff --git a/dom/ipc/ContentProcessChild.cpp b/dom/ipc/ContentProcessChild.cpp

>+bool
>+ContentProcessChild::RecvNotifyVisited(const nsCString& aURISpec, 
>+                                       const bool& mIsVisited)
>+{
>+    nsresult rv;
>+
>+    // reconstruct our IPDL-passed nsIURI
>+    nsCOMPtr<nsIURI> newURI;
>+    rv = NS_NewURI(getter_AddRefs(newURI), aURISpec);
>+    // Our failure mode is to consider the link unvisited.
>+    if (NS_SUCCEEDED(rv)) {
>+        History *hs = History::GetSingleton();
>+        if (hs) {
>+            hs->NotifyVisited(newURI, mIsVisited);

Need to rename mIsVisited to aIsVisited. (Though I don't understand the change to NotifyVisited to take an isVisited arg; I didn't look carefully.)

GetSingleton can't fail anymore; no need to check it.

Once bug 541174 is done, the URI serialization here can be improved, and then nothing here should be able to fail.

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp

>@@ -170,6 +178,14 @@ ContentProcessParent::Observe(nsISupport

>+    if (!strcmp(aTopic, HISTORY_NOTIFY_VISITED_TOPIC) && mSubprocess) {
>+      NS_ConvertUTF16toUTF8 dataStr(aData);
>+      const char *isVisited = dataStr.get();
>+      nsCString uriSpec;
>+      ((nsIURI*)aSubject)->GetSpec(uriSpec);
>+      SendNotifyVisited(uriSpec, !strcmp(isVisited, "true") ? true : false);

This could be improved: in particular, you need a QI to get from nsISupports to nsIURI.

>+bool
>+ContentProcessParent::RecvStartVisitedQuery(const nsCString& aURISpec, nsresult* rv)
>+{
>+    // reconstruct our IPDL-passed nsIURI
>+    nsCOMPtr<nsIURI> newURI;
>+    *rv = NS_NewURI(getter_AddRefs(newURI), aURISpec);
>+    if (NS_SUCCEEDED(*rv)) {
>+        nsCOMPtr<IHistory> history = do_GetService(NS_IHISTORY_CONTRACTID);
>+        if (history) {
>+            *rv = history->RegisterVisitedCallback(newURI, nsnull);
>+        }
>+    } else {
>+        *rv = NS_ERROR_UNEXPECTED; 
>+    }  
>+    return true;
>+}

This thing should be asynchronous, I think, and errors need not be propagated back. Is there a reason you made it sync?

>diff --git a/dom/ipc/PContentProcess.ipdl b/dom/ipc/PContentProcess.ipdl

>+    async NotifyVisited(nsCString aURISpec, bool mIsVisited);

No need for the 'async' here, that's the default.
Thanks for your comments, Dan.
As soon as Stechz's patch in bug 439094 gets r+, I'll correct this one and re-post for review.
forget my last comment, it should have read :

Thanks for your comments, Dan.
As soon as Stechz's patch in bug 556400 gets r+, I'll correct this one and
re-post for review.
Attachment #435189 - Flags: feedback?(dwitte)
Attached patch patch v.4 (obsolete) — Splinter Review
Assignee: bugzillaFred → dougt
Attachment #435189 - Attachment is obsolete: true
Attached patch patch v.4 (obsolete) — Splinter Review
Attachment #447179 - Flags: review?
Attachment #447179 - Flags: review? → review?(sdwilsh)
Comment on attachment 447179 [details] [diff] [review]
patch v.4

per in-person discussion with dougt
Attachment #447179 - Flags: review?(sdwilsh) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #447179 - Attachment is obsolete: true
Attachment #447635 - Flags: review?(sdwilsh)
Attachment #447635 - Flags: review?(sdwilsh) → review?(jones.chris.g)
Comment on attachment 447635 [details] [diff] [review]
patch

Just FTR, I was told there's a carte-blanche r+ on the history API stuff.

>diff --git a/dom/ipc/Makefile.in b/dom/ipc/Makefile.in
>--- a/dom/ipc/Makefile.in
>+++ b/dom/ipc/Makefile.in
>@@ -71,16 +71,17 @@ CPPSRCS = \
>   $(NULL)
> 
> include $(topsrcdir)/config/config.mk
> include $(topsrcdir)/ipc/chromium/chromium-config.mk
> include $(topsrcdir)/config/rules.mk
> 
> LOCAL_INCLUDES += \
> 		-I$(srcdir)/../../content/base/src \
>+		-I$(srcdir)/../../toolkit/components/places/src \

You shouldn't need this given that you're now exporting History.h through mozilla/places.

>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
>@@ -37,16 +37,21 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifdef MOZ_IPC
> #include "mozilla/dom/ContentProcessChild.h"
> #include "nsXULAppAPI.h"
> #endif
> 
>+#ifdef MOZ_IPC
>+#include "mozilla/dom/ContentProcessChild.h"
>+#include "mozilla/dom/ContentProcessParent.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"
>@@ -59,16 +64,17 @@ namespace mozilla {
> namespace places {
> 
> ////////////////////////////////////////////////////////////////////////////////
> //// Global Defines
> 
> #define URI_VISITED "visited"
> #define URI_NOT_VISITED "not visited"
> #define URI_VISITED_RESOLUTION_TOPIC "visited-status-resolution"
>+#define HISTORY_NOTIFY_VISITED_TOPIC "history-notify-visited"

I'm not sure why we need both the "query-resolved" and "is-visited"
topics, but I defer to the carte-blanche r+ on history API.

> NS_IMETHODIMP
> History::RegisterVisitedCallback(nsIURI* aURI,
>                                  Link* aLink)
> {
>   NS_ASSERTION(aURI, "Must pass a non-null URI!");
>-  NS_ASSERTION(aLink, "Must pass a non-null Link object!");
> 

It appears to me from a cursory glance that you could have re-used
|Link|s for remote history queries here and avoided the extra
observer-service topic and the change in semantics of this method, but
again I'll defer to carte-blanche r+ on History stuff.

>   // First, ensure that our hash table is setup.
>   if (!mObservers.IsInitialized()) {
>     NS_ENSURE_TRUE(mObservers.Init(), NS_ERROR_OUT_OF_MEMORY);
>   }
> 
>   // Obtain our array of observers for this URI.
> #ifdef DEBUG
>   bool keyAlreadyExists = !!mObservers.GetEntry(aURI);
> #endif
>   KeyClass* key = mObservers.PutEntry(aURI);
>   NS_ENSURE_TRUE(key, NS_ERROR_OUT_OF_MEMORY);
>   ObserverArray& observers = key->array;
> 
>-  if (observers.IsEmpty()) {
>-    NS_ASSERTION(!keyAlreadyExists,
>-                 "An empty key was kept around in our hashtable!");
>+  nsresult rv = NS_OK;
>+#ifdef MOZ_IPC
>+  // If we are a content process, always remote the request to the
>+  // parent process.
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    mozilla::dom::ContentProcessChild * cpc = 
>+      mozilla::dom::ContentProcessChild::GetSingleton();

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

>+    NS_ASSERTION(cpc, "Content Protocol is NULL!");
>+    cpc->SendStartVisitedQuery(IPC::URI(aURI));

It appears to me that you're going to send this message for every call
to RegisterVisitedCallback(), which is sort of different than the
existing logic which only does something expensive (DB ops) if this is
the "first time" the visited-ness is queried.  If you were using Links
to track remote history queries, it would be easy to preserve that
optimization.  That said, I'm OK with this until it shows up on
profiles.

>+  }
>+  else
>+#endif
>+  rv = VisitedQuery::Start(aURI);
> 
>-    // We are the first Link node to ask about this URI, or there are no pending
>-    // Links wanting to know about this URI.  Therefore, we should query the
>-    // database now.

sdwilsh might be upset about this comment being toasted.

r+ for the subset of this I'm qualified to review.
Attachment #447635 - Flags: review?(jones.chris.g) → review+
(In reply to comment #33)
> Just FTR, I was told there's a carte-blanche r+ on the history API stuff.
Well, I still need to look it over, but mak has looked at it.

> It appears to me from a cursory glance that you could have re-used
> |Link|s for remote history queries here and avoided the extra
> observer-service topic and the change in semantics of this method, but
> again I'll defer to carte-blanche r+ on History stuff.
I was going along this thought process too.  Doug, what was the reason we didn't go this route again?

> >-    // We are the first Link node to ask about this URI, or there are no pending
> >-    // Links wanting to know about this URI.  Therefore, we should query the
> >-    // database now.
> 
> sdwilsh might be upset about this comment being toasted.
Well, that's because we aren't using Link, but I'd like to keep that invariant if we can.
(In reply to comment #34)
> (In reply to comment #33)
> > >-    // We are the first Link node to ask about this URI, or there are no pending
> > >-    // Links wanting to know about this URI.  Therefore, we should query the
> > >-    // database now.
> > 
> > sdwilsh might be upset about this comment being toasted.
> Well, that's because we aren't using Link, but I'd like to keep that invariant
> if we can.

The |rv = VisitedQuery::Start(aURI);| call was moved but its comment didn't come with it.
I followed what fred23 started on.  The code is pretty clean not having two "kinds" of Link objects -- one for chrome and one for content.
(In reply to comment #36)
> I followed what fred23 started on.  The code is pretty clean not having two
> "kinds" of Link objects -- one for chrome and one for content.
Well, Link is just an interface really.  We already have at least two implementations in the tree.
do you think the code will be easier to read if there was another implementation of Link in the tree?
(In reply to comment #38)
> do you think the code will be easier to read if there was another
> implementation of Link in the tree?
Well, History.cpp will be, and we won't have to throw away one of our invariants for content processes.
what would the Link implementation do, exactly?
Attached patch without the observer (obsolete) — Splinter Review
okay, reviewing the observer is a good thing.  however, i don't see a good way to have a fake Link implementation for just the parent.  So, this patch ignores that request, and cleans up the observer stuff.  Please take a look...

also, i needed to keep the makefile bits to avoid complication errors.
Attachment #449035 - Flags: review?(sdwilsh)
Comment on attachment 449035 [details] [diff] [review]
without the observer

>+++ b/toolkit/components/places/src/History.cpp
> #ifdef MOZ_IPC
> #include "mozilla/dom/ContentProcessChild.h"
> #include "nsXULAppAPI.h"
> #endif
> 
>+#ifdef MOZ_IPC
>+#include "mozilla/dom/ContentProcessChild.h"
>+#include "mozilla/dom/ContentProcessParent.h"
>+#endif
Er, combine these and get rid of the duplicates please

> void
> History::NotifyVisited(nsIURI* aURI)
> {
>   NS_ASSERTION(aURI, "Ruh-roh!  A NULL URI was passed to us!");
> 
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Default) {
>+    mozilla::dom::ContentProcessParent * cpp = 
>+      mozilla::dom::ContentProcessParent::GetSingleton();
>+    NS_ASSERTION(cpp, "Content Protocol is NULL!");
>+    (void) cpp->SendNotifyVisited(IPC::URI(aURI));
>+  }
>+#endif
nits: no space after type, before |*|
no space after |(void)|

> NS_IMETHODIMP
> History::RegisterVisitedCallback(nsIURI* aURI,
>                                  Link* aLink)
> {
>   NS_ASSERTION(aURI, "Must pass a non-null URI!");
>-  NS_ASSERTION(aLink, "Must pass a non-null Link object!");
This only gets relaxed in IPC builds, right?  Also, you need to update the IHistory documentation for this (and this is an interface behavior change so this will need a revd cid on IHistory, plus the corresponding sr [likely from bz] which should be simple).

>-  if (observers.IsEmpty()) {
>-    NS_ASSERTION(!keyAlreadyExists,
>-                 "An empty key was kept around in our hashtable!");
Not clear to me why this is removed.  This code was specifically added in http://hg.mozilla.org/mozilla-central/rev/37af1321c320 to make sure we aren't leaking observer arrays which don't show up on tinderbox.

>+  nsresult rv = NS_OK;
>+#ifdef MOZ_IPC
>+  // If we are a content process, always remote the request to the
>+  // parent process.
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    mozilla::dom::ContentProcessChild * cpc = 
>+      mozilla::dom::ContentProcessChild::GetSingleton();
>+    NS_ASSERTION(cpc, "Content Protocol is NULL!");
>+    cpc->SendStartVisitedQuery(IPC::URI(aURI));
I've thought about this a bunch, and I think it's best if this block of code (the whole ifdef really) was just moved into VisitedQuery::Start.  I think we had discussed this, and I was on the fence.

>-    // We are the first Link node to ask about this URI, or there are no pending
>-    // Links wanting to know about this URI.  Therefore, we should query the
>-    // database now.
>-    nsresult rv = VisitedQuery::Start(aURI);
>-    if (NS_FAILED(rv)) {
>-      // Remove our array from the hashtable so we don't keep it around.
>-      mObservers.RemoveEntry(aURI);
>-      return rv;
>-    }
And then all of this can stay (including the important comment that got completely nuked in your patch).

>+  // We are done.
>+  if (!aLink)
>+    return NS_OK;
nit: brace one line ifs.
This should only be true in IPC code, right?

>+EXPORTS_mozilla/places = \
>+  History.h \
>+  $(NULL)
Concerned about exporting this, but I don't see how else you could really accomplish what you want without doing so.  *sigh*

r- for the observer array leak issue.  Once that is fixed, I suspect this will get r+.
Attachment #449035 - Flags: review?(sdwilsh) → review-
Attached patch patch v.4 (WIP!) (obsolete) — Splinter Review
Attachment #423103 - Attachment is obsolete: true
Attachment #423516 - Attachment is obsolete: true
Attachment #447635 - Attachment is obsolete: true
Attachment #449035 - Attachment is obsolete: true
Attachment #450008 - Flags: feedback?(sdwilsh)
Comment on attachment 450008 [details] [diff] [review]
patch v.4 (WIP!)

this doesn't work
Attachment #450008 - Flags: feedback?(sdwilsh) → feedback-
Attached patch patch v.5 (obsolete) — Splinter Review
bz, please sr the change to the ihistory interface.  I modified a precondition allowing no Link pointer in the IPC parent process.
Attachment #450008 - Attachment is obsolete: true
Attachment #450209 - Flags: superreview?(bzbarsky)
Attachment #450209 - Flags: review?(sdwilsh)
(In reply to comment #42)
> >-  if (observers.IsEmpty()) {
> >-    NS_ASSERTION(!keyAlreadyExists,
> >-                 "An empty key was kept around in our hashtable!");
> Not clear to me why this is removed.  This code was specifically added in
> http://hg.mozilla.org/mozilla-central/rev/37af1321c320 to make sure we aren't
> leaking observer arrays which don't show up on tinderbox.
This particular assertion was to actually make sure we remove the array when we return early so we don't end up with arrays sitting around forever and ever until shutdown.  In your code, you had this:
>  // We are done.
>  if (!aLink)
>    return NS_OK;
which doesn't remove the array on an early return, so it was sitting around.  This means that you'd end up with an empty array the second time you tried that URI since in the remote case we'd never call Unregister.

So, we can keep the if block because observers.IsEmpty() will be accurate in both processes as long as after we call VisitedQuery::Start we remove the observer array if NS_FAILED(rv) *or* aLink is NULL (basically just change that existing if right after it).
Comment on attachment 450209 [details] [diff] [review]
patch v.5

>+     * @pre aLink may be null only in the MOZ_IPC parent process
nit: period at the end of the sentence please.  Also, do you mean the default process here?

>@@ -918,45 +940,48 @@ History::VisitURI(nsIURI* aURI,
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> History::RegisterVisitedCallback(nsIURI* aURI,
>                                  Link* aLink)
> {
>   NS_ASSERTION(aURI, "Must pass a non-null URI!");
>-  NS_ASSERTION(aLink, "Must pass a non-null Link object!");
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    NS_ASSERTION(aLink, "Must pass a non-null URI!");
>+  }
>+#endif
> 
>   // First, ensure that our hash table is setup.
>   if (!mObservers.IsInitialized()) {
>     NS_ENSURE_TRUE(mObservers.Init(), NS_ERROR_OUT_OF_MEMORY);
>   }
> 
>   // Obtain our array of observers for this URI.
> #ifdef DEBUG
>   bool keyAlreadyExists = !!mObservers.GetEntry(aURI);
> #endif
>   KeyClass* key = mObservers.PutEntry(aURI);
>   NS_ENSURE_TRUE(key, NS_ERROR_OUT_OF_MEMORY);
>   ObserverArray& observers = key->array;
> 
>-  if (observers.IsEmpty()) {
>-    NS_ASSERTION(!keyAlreadyExists,
>-                 "An empty key was kept around in our hashtable!");
>+  // We are the first Link node to ask about this URI, or there are no pending
>+  // Links wanting to know about this URI.  Therefore, we should query the
>+  // database now.
>+  nsresult rv = VisitedQuery::Start(aURI);
>+  if (NS_FAILED(rv)) {
>+    // Remove our array from the hashtable so we don't keep it around.
>+    mObservers.RemoveEntry(aURI);
>+    return rv;
>+  }
> 
>-    // We are the first Link node to ask about this URI, or there are no pending
>-    // Links wanting to know about this URI.  Therefore, we should query the
>-    // database now.
>-    nsresult rv = VisitedQuery::Start(aURI);
>-    if (NS_FAILED(rv)) {
>-      // Remove our array from the hashtable so we don't keep it around.
>-      mObservers.RemoveEntry(aURI);
>-      return rv;
>-    }
>-  }
>+  // We are done.
>+  if (!aLink)
>+    return NS_OK;
> 
>   // Sanity check that Links are not registered more than once for a given URI.
>   // This will not catch a case where it is registered for two different URIs.
>   NS_ASSERTION(!observers.Contains(aLink),
>                "Already tracking this Link object!");
> 
>   // Start tracking our Link.
>   if (!observers.AppendElement(aLink)) {
This whole block should actually look like this (per comment 46):

@@ -235,8 +235,14 @@ NS_IMETHODIMP
 History::RegisterVisitedCallback(nsIURI* aURI,
                                  Link* aLink)
 {
-  NS_ASSERTION(aURI, "Must pass a non-null URI!");
-  NS_ASSERTION(aLink, "Must pass a non-null Link object!");
+  NS_PRECONDITION(aURI, "Must pass a non-null URI!");
+#ifdef MOZ_IPC
+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
+    NS_PRECONDITION(aLink, "Must pass a non-null URI!");
+  }
+#else
+  NS_PRECONDITION(aLink, "Must pass a non-null URI!");
+#endif

   // First, ensure that our hash table is setup.
   if (!mObservers.IsInitialized()) {
@@ -259,7 +265,7 @@ History::RegisterVisitedCallback(nsIURI*
     // Links wanting to know about this URI.  Therefore, we should query the
     // database now.
     nsresult rv = VisitedQuery::Start(aURI);
-    if (NS_FAILED(rv)) {
+    if (NS_FAILED(rv) || !aLink) {
       // Remove our array from the hashtable so we don't keep it around.
       mObservers.RemoveEntry(aURI);
       return rv;

r=sdwilsh with these changes
Attachment #450209 - Flags: review?(sdwilsh) → review+
The assert text in History::RegisterVisitedCallback is wrong.

I also don't understand why the observers.IsEmpty() check has gone away and why the outdented comment there makes sense.  Can someone explain it to me?
(In reply to comment #48)
> The assert text in History::RegisterVisitedCallback is wrong.
Wow, not sure how I missed that :(

> I also don't understand why the observers.IsEmpty() check has gone away and why
> the outdented comment there makes sense.  Can someone explain it to me?
It hasn't gone away (see comment 47).  Fennec is a bit buggy, and that was leaving dougt to believe that the patch was wrong, but we've determined that it is, in fact, correct with the changes I've outlined.
OK.  I'd really like to see the patch with your proposed changes merged in before I sr, then....  The interface change itself looks fine, though.
Attached patch patch v.6 (obsolete) — Splinter Review
merges sdwlish's comments.
Attachment #450209 - Attachment is obsolete: true
Attachment #450668 - Flags: superreview?(bzbarsky)
Attachment #450209 - Flags: superreview?(bzbarsky)
Attachment #450668 - Flags: superreview?(bzbarsky) → superreview+
http://hg.mozilla.org/projects/electrolysis/rev/ca18a037097f
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
this was backed out a few days ago because the required places patches were backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v6.1Splinter Review
just the patch Doug pushed to tryserver, that applies.
Attachment #450668 - Attachment is obsolete: true
This patch is what is causing the leak on tryserver, and exactly this part:
 mozilla::dom::ContentProcessParent::GetSingleton();

we create History singleton in the content process, everything works fine but looks like its destructor is never called, as if the content process is just killed. It i run a mochitest like test_visited_reftests.html this code path is touched and we leak History from the content process, or at least the mochitest harness is complaining about that.
Depends on: 556400
No longer depends on: async-visited-check
tracking-fennec: --- → 2.0+
We think that maybe services aren't being cleaned up.  Chris Jones recommended we start playing in the debugger around here:

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentProcessChild.cpp#156
in the patch we are doing something like:

History::GetSingleton()->NotifyVisited(newURI);


Doesn't History::GetSingleton() addref?  If so, that would leak, right?
Yep.  It does and it does.  Why is that stuff not returning already_AddRefed, exactly?  :(
(In reply to comment #58)
> Yep.  It does and it does.  Why is that stuff not returning already_AddRefed,
> exactly?  :(
Because it's what we pass to the constructor in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesModule.cpp#23

I don't think it will take an already_AddRefed
> I don't think it will take an already_AddRefed

Well, we should fix that crap!
With latest patch my DEBUG build aborting with message:
All IPDL URIs must be serializable or an allowed scheme: 'scheme.EqualsASCII("about:") || scheme.EqualsASCII("javascript:")', file ../../dist/include/mozilla/net/NeckoMessageUtils.h, line 89

While loading http://hs.fi
I would appreciate a rebased version of this patch.
see: http://hg.mozilla.org/users/dougt_mozilla.com/wip/

The first two patches are related to places stuff.  I can attach (if you don't do before then) tomorrow from work.
The scheme turns out to be "javascript" in the case of hs.fi.  I grow less and less fond of this serialization solution with every exception I add.
Note that serializing javascript: URIs in general (as in, if one plans to load them somewhere) is a huge pain, since they carry a lot of state.  Of course none of that state matters for history....
jdm - how about just dropping this special case stuff.  If a URI support serialization, use it.  The default could just be to NS_WARN.  The special case code could just silence the warn for a given scheme after we verify it is okay to use.
Depends on: 582442
http://hg.mozilla.org/mozilla-central/rev/9ebd1145c98a
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Comment on attachment 457430 [details] [diff] [review]
patch v6.1

>-  NS_ASSERTION(aLink, "Must pass a non-null Link object!");
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    NS_PRECONDITION(aLink, "Must pass a non-null URI!");
>+  }
>+#else
>+  NS_PRECONDITION(aLink, "Must pass a non-null URI!");
>+#endif
>+
> 
How did aLink suddenly become a URI object?
(Also there's a spurious extra blank line.)
looks like an assertion description mistake.
(In reply to comment #68)
> How did aLink suddenly become a URI object?
> (Also there's a spurious extra blank line.)
Gwaa, I thought I pointed that out (remember seeing it), but I clearly didn't.  Doug, can you please land the fix for that comment?
You need to log in before you can comment on or make changes to this bug.