This is a very small and hopefully manageable part of "hooking up docshell and progres notifications": when the user navigates from one page to the next in the tab browser (such as clicking on a link or submitting a form), it should inform chrome of the change. Bonus (could be put off for another bug): chrome should have the opportunity to say "no, don't do that navigation" and perhaps redirect the navigation to another content process.

Chrome script currently uses nsIWebProgressListener to keep aware of changes: http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl through the <xul:browser>.webProgress property, see https://developer.mozilla.org/en/XUL/browser#p-webNavigation

Comment #1, by bsmedberg, was extremely useful in helping me to understand this development task. In case anyone is confused, the second link, above, should have been https://developer.mozilla.org/en/XUL/browser#p-webProgress

(Long comment. Do not read if subsequent comments make this one irrelevant. Check below.) The task is to arrange for the content process to inform the chrome process of progress during navigation. Currently, this progress notification occurs as an interaction between an instance of nsIWebProgress and instances of nsIWebProgressListener. The listeners (which implement nsIWebProgressListener) register their interest with the browser object (which implements nsIWebProgress). So, we can accomplish the task by arranging that instances of nsIWebProgress that are in the content process can talk to the instances of nsIWebProgressListeners that may be in the chrome process. That is how I interpret the task; if this is not correct, then the rest of this comment won't be right. BTW, only the nsDocLoader class implements the nsIWebProgress functionality. The member functions of nsIWebProgessListener implementers look like this: void OnStateChange(nsIWebProgress *aWebProgress, nsIRequest *aRequest, PRUint32 aStateFlags, nsresult aStatus); When an appropriate state change occurs, the nsIWebProgress notifies the nsIWebProgressListener by invoking the above function on it. The obvious thing to do, to make this work cross-process, is to create an IPDL protocol which can send messages that correlate directly with the functions of nsIWebProgressListeners. So, we might have: protocol PIWebProgressListener { parent: StateChange( /* ??? */ ); } But, notice the pointer to the nsIRequest object in the signature for OnStateChange. The listener (which is likely to be in the chrome process) must have access to the nsIRequest object (likely in the content process); this is how the listener finds out about the request whose state has changed. (Ignore the nsIWebProgress pointer, for now.) How can we send the nsIRequest object as an argument to the StateChange() message? We can't. We cannot serialize it and send it, because then there are 2 nsIRequest objects, which is just plain wrong. We can't create a null nsIRequest (whose methods do nothing) in the chrome process, because the whole reason for the StateChange notification might be to, say, cancel() the request, which would be impossible with a null nsIRequest. The solution is to proxy the nsIRequest in the chrome process. So, method invocations on the proxy are translated (via another IPDL protocol) into method invocations on the real object in the content process. I wonder if I'm on the right track, here, because this kind of proxying mechanism is complex to create. It requires ensuring that the lifetimes of the object proxied and the proxy itself are synchronized. Also, there must be some way to destroy the proxy when it is not used, without requiring the using code to be complicit in reference counting. However, once this kind of complexity is dealt with, it is easily retargeted to other interfaces. I'm happy to code this, but I wonder why it hasn't already been done or planned. That's why I wonder if I'm on the right track with my solution to this task. Comments? BTW, the task would require proxying more than just nsIRequests - it also would require nsIURI proxies, and nsIWebProgress proxies. The latter is easier, since the TabParent could easily be transformed into a nsIWebProgress proxy.

In the short term we do not need to do anything with the nsIRequest: just throw it away on the child side and pass null to the nsIWebProgressListener. We can revisit what the actual final API should look like later. (This isn't it!) As for protocols, I don't think you need to have a new one for this. Just send the progress messages to PIFrameEmbedding. The implementation of PIFrameEmbedding can then keep a local list of interested listeners.

Longer-term, when necko is on the parent side anyway, that's where the nsIRequest will live too. So things will Just Work then, right?

W.I.P. only. No review necessary.

Work in progress. I have set up some messages (notifyStateChange, notifyProgressChange, and so on) that get sent to chrome process when content progress does navigation. TabParent is set up to notify listeners when this happens, although I haven't figured out which listeners those should be yet. Right now it just printf's that it got the messages. Next step is to hook this up to <xul:browser>.webProgress, so that when clients add listeners to browser.webProgress they get added to the TabParent. Not sure how to do this yet.

From the necko e10s discussion we had today, it sounds like some weblisteners are going to need a pointer to the nsIRequest, and that this will need to be to the underlying nsHttpChannel that lives in chrome (not the HttpChannelChild that lives on the child). It sounds like we may need to provide some sort of accessor function in HttpChannelChild that lets the WebProgress (on the child) get a pointer address for the nsHttpChannel and pass it via IPC to the chrome WebProgressListener. (We'll also need to guarantee that the nsHttpChannel is still alive). But I don't know enough about the use cases here to say if this is the right solution. In particular, if the current weblistener API is synchronous, and clients expect to be able to do something to the nsIRequest (like cancel, redirect, whatever), then behavior might be quite different, as the request will not be blocked and may have moved along by the time the webprogresslistener runs. But if the fix above does sound right, let me know and we can implement it for you.

My takeaway from the meeting was "we don't know whether weblisteners in chrome will need access to the nsIRequest, and bz's going to make a list". In any case we know we *don't* need it for the Fennec UI, so let's not get too sidetracked on that point.

It actually looks like we're pretty good. I made a summary of existing progress listeners at https://wiki.mozilla.org/Electrolysis/ProgressListeners (not linked from anywhere yet). Most consumers don't use the request at all. The exceptions are: nsLoginManager.js: Uses aRequest.name for logging only. browser.js: Uses aRequest only to get URI. updates.xml: Checks aRequest for requestSucceeded for HTTP. Plus whatever part of the C++ we want to put in the chrome, of course; it looks like nsSecureBrowserUIImpl is going to be the only issue there.

At the moment, I can say that if I have a mirror of load group structures and potentially also docshells (fake objects) on chrome process, security UI might live on the chrome process and have synchronously all info from the real channels.

This works locally (Linux) and builds on OSX, Windows. Maybe need to test on OSX as well, since there's a small codepath change there. Try-server is reporting failed mobile builds, but try-server looks kind of wonky in general today and everyone's mobile builds seem to be failing.

I assume there was some reason to cancel the review request.

Sorry, yes, looking at some test failures that I hadn't noticed before.

Comment on attachment 429630 [details] [diff] [review] implements browser.webProgress for remote browsers, plus test Re-requesting review, can't reproduce tryserver failures on either Linux or OSX locally.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1267577531.1267588173.23224.gz#err8 is the last failure I was looking at. The first test seems unrelated and I can't reproduce, the other failures seemed to be happening on other tryserver builds as well. Damon asked me to try to post an estimation of time to completion, so here goes... Initial review: 1 day, Address review comments, re-review: 3 days Wait for someone to check in for me: 2 days?

If you explicitly ask me to check the final patch in, I'll do that in the same day or next morning (EET).

from review, smaug says: I wonder if all these messages should be sync. Need to discuss with bsmedberg and bz about this. And check what the current webprogresslisteners need. "Messages" here being status change, location change, and so on. From bz's comment 10, seems like no current clients need sync calls, if they are just getting information from the request? What do you guys think about this?

I think we should start with async, but be aware of cases where it might actually matter (like the URL bar getting out of sync with content) and perhaps revisit or do some of the messages more synchronously later.

I was wondering whether we need to support webProgressListener2, which includes onProgressChange64 and onRefreshAttempted. onProgressChange64 is trivial, obviously, but onRefreshAttempted allows listeners to cancel the refresh. At least one client, browser.js, does sometimes cancel the refresh. Seems like options are: a) don't support webProgressListener2 b) support, but silently drop onRefreshAttempted notifications (ie don't pass them to listeners.) c) support, pass along notifications, ignore return value (False is supposed to mean 'cancel the request') d) support, do this message synchronously.

Support, but don't do the request until we get a yes/no answer?

Seems like that would require either a blocking (synchronous) onRefreshAttempted call or a new API for nsIWebProgress, though. Something like: call onRefreshAttempted to listeners, idle/yield, do refresh when all listeners explicitly call refresh().

We control the only caller of onRefreshAttempted. So yes, I'm proposing changing the API for the caller (which is in the content process and fully under our control). We don't have to change the API for things that implement the method (which are almost certainly all chrome), as far as I can tell: whatever code bridges from content to chrome can handle doing that. Or we can make it sync, yes. That would be somewhat less work, I guess. Oprions a, b, c from comment 21 would allow refreshes that shouldn't be allowed; this potentially leads to security bugs, last I checked.

Want to look this over a little, will r? when ready.

I think it will be hard to implement isLoadingDocument correctly. The obvious approach would be to set the bit to true on loadURI, and flip it off when we get STATE_STOP notification. However, loadURI can fail silently (as in, even the docShell in the child process returns NS_OK). Another complication is, what if we send two loadURIs close together? We can't tell their onStateChange notifications apart, because we don't get the the nsIRequest object. I have it returning NOT_IMPLEMENTED right now, please advise if the functionality is necessary. It does have one caller in browser.js. Fixed other review comments, now sends notifications to nsIWebProgressListener2 when possible. Wasn't able to test onRefreshAttempted functionality, since it seems we haven't implemented nsIWebNavigation reload() yet.

(In reply to comment #26) > Wasn't able to test onRefreshAttempted functionality, since it > seems we haven't implemented nsIWebNavigation reload() yet. Couldn't you use messageManager.loadFrameScript to reload the page? docShell is available in TabChildGlobal, and docShell implements nsIWebNavigation

Comment on attachment 432694 [details] [diff] [review] addresses review comments >diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp >--- a/dom/ipc/ContentProcessParent.cpp >+++ b/dom/ipc/ContentProcessParent.cpp >@@ -165,23 +165,29 @@ > mSubprocess = nsnull; > } > return NS_OK; > } > > PIFrameEmbeddingParent* > ContentProcessParent::AllocPIFrameEmbedding() > { >- return new TabParent(); >+ TabParent* parent; >+ NS_NEWXPCOM(parent, TabParent); Why this change? > bool > ContentProcessParent::DeallocPIFrameEmbedding(PIFrameEmbeddingParent* frame) > { >- delete frame; >+ TabParent* parent = (TabParent*) frame; Use C++ casting, static_cast<TabParent*>(frame) >diff --git a/dom/ipc/PIFrameEmbedding.ipdl b/dom/ipc/PIFrameEmbedding.ipdl >--- a/dom/ipc/PIFrameEmbedding.ipdl >+++ b/dom/ipc/PIFrameEmbedding.ipdl >@@ -61,16 +61,33 @@ > /** > * When child sends this message, parent should move focus to > * the next or previous focusable element. > */ > moveFocus(bool forward); > > sendEvent(RemoteDOMEvent aEvent); > >+ notifyStateChange(PRUint64 stateFlags, nsresult status); Why PRUint64? nsIWebProgressListener uses 32 bits. >@@ -116,29 +118,33 @@ > > nsCOMPtr<nsIWebBrowser> webBrowser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID); > if (!webBrowser) { > NS_ERROR("Couldn't create a nsWebBrowser?"); > return NS_ERROR_FAILURE; > } > > webBrowser->SetContainerWindow(this); >+ nsCOMPtr<nsIWeakReference> weak >+ = do_GetWeakReference((nsSupportsWeakReference*) this); C++ casting, please. Indentation should be 2 spaces. >+NS_IMETHODIMP >+TabChild::OnProgressChange(nsIWebProgress *aWebProgress, >+ nsIRequest *aRequest, >+ PRInt32 aCurSelfProgress, >+ PRInt32 aMaxSelfProgress, >+ PRInt32 aCurTotalProgress, >+ PRInt32 aMaxTotalProgress) >+{ >+ SendnotifyProgressChange(aCurSelfProgress, aMaxSelfProgress, >+ aCurTotalProgress, aMaxTotalProgress); >+ return NS_OK; >+} ... >+NS_IMETHODIMP >+TabChild::OnProgressChange64(nsIWebProgress *aWebProgress, >+ nsIRequest *aRequest, >+ PRInt64 aCurSelfProgress, >+ PRInt64 aMaxSelfProgress, >+ PRInt64 aCurTotalProgress, >+ PRInt64 aMaxTotalProgress) >+{ >+ SendnotifyProgressChange(aCurSelfProgress, aMaxSelfProgress, >+ aCurTotalProgress, aMaxTotalProgress); >+ return NS_OK; >+} So only one of these will be called, right? Could you actually document it somewhere here. >+TabParent::RecvnotifyStateChange(const PRUint64& aStateFlags, >+ const nsresult& aStatus) >+{ >+ /* >+ * First notify any listeners of the new state info... >+ * >+ * Operate the elements from back to front so that if items get >+ * get removed from the list it won't affect our iteration >+ */ >+ nsCOMPtr<nsIWebProgressListener> listener; >+ PRInt32 count = mListenerInfoList.Length(); >+ >+ while (--count >= 0) { >+ TabParentListenerInfo *info = &mListenerInfoList[count]; >+ if (!(info->mNotifyMask & (aStateFlags >>16))) { Please explain >>16. And there should be a space before 16. Would be better to have the 16 as a #define. >+ >+NS_IMETHODIMP >+TabChild::OnRefreshAttempted(nsIWebProgress *aWebProgress, >+ nsIURI *aURI, PRInt32 aMillis, >+ PRBool aSameURL, PRBool *aRefreshAllowed) { New line after ')' and { to the beginning of the new line. >+ >+ mListenerInfoList.Compact(); This is in many places. Why? Could you just Compact when a listener is removed and the size of the list a significantly larger than the number of items in it? Or are you trying to remove the possible non-valid weakrefs? I doubt Compact() is enough for that. >+nsresult >+TabParent::AddProgressListener(nsIWebProgressListener* aListener, >+ PRUint32 aNotifyMask) >+{ >+ nsresult rv; >+ >+ TabParentListenerInfo* info = GetListenerInfo(aListener); >+ if (info) { >+ // The listener is already registered! >+ return NS_ERROR_FAILURE; >+ } >+ >+ nsWeakPtr listener = do_GetWeakReference(aListener); >+ if (!listener) { >+ return NS_ERROR_INVALID_ARG; >+ } >+ >+ info = new TabParentListenerInfo(listener, aNotifyMask); >+ if (!info) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } >+ >+ rv = mListenerInfoList.AppendElement(*info) ? NS_OK : NS_ERROR_FAILURE; So if AppendElement fails, you leak info. Could you use nsAutoPtr and its forget() method here? >+NS_IMETHODIMP >+TabParent::RemoveProgressListener(nsIWebProgressListener *aListener) >+{ >+ nsresult rv; >+ >+ TabParentListenerInfo* info = GetListenerInfo(aListener); >+ if (info) { >+ rv = mListenerInfoList.RemoveElement(*info) ? NS_OK : NS_ERROR_FAILURE; >+ delete info; >+ } else { >+ // The listener is not registered! >+ rv = NS_ERROR_FAILURE; >+ } >+ return rv; Perhaps something like. nsAutoPtr<TabParentListenerInfo> info = GetListenerInfo(aListener); return info && mListenerInfoList.RemoveElement(*info) ? NS_OK : NS_ERROR_FAILURE; >-class TabParent : public PIFrameEmbeddingParent >+struct TabParentListenerInfo { '{' should be in the next line. Although the comments are mainly just nits, I could re-read the patch still once.

Fixed above comments, except: >>+ notifyStateChange(PRUint64 stateFlags, nsresult status); > Why PRUint64? > nsIWebProgressListener uses 32 bits. This lets me use one notification to TabParent when TabChild receives a notification. Then, TabParent will call listeners either with 64-bit or 32-bit parameters, depending on whether TabParent's listeners are webProgressListener2 or not. TabChild's incoming notifications all _should_ be 64-bit, since it implements webProgressListener2, although docLoader doesn't actually respect that right now. >> >+ rv = mListenerInfoList.AppendElement(*info) ? NS_OK : NS_ERROR_FAILURE; >So if AppendElement fails, you leak info. Could you use nsAutoPtr and its >forget() method here? I modified my code to not leak in the failure case now, but it seemed cleaner without nsAutoPtr.

smaug points out that the state flags don't need 64 bits, even if the progress numbers might.

Comment on attachment 433448 [details] [diff] [review] 32-bit state flags >+NS_IMETHODIMP >+nsFrameLoader::GetWebProgress(nsIWebProgress **webProgress) Change the parameter name to aWebProgress. >+{ >+ nsresult rv; *aWebProgress = nsnull; >+#ifdef MOZ_IPC >+ if (mRemoteFrame) { >+ if (!mChildProcess) { >+ TryNewProcess(); >+ } >+ if (!mChildProcess) { >+ return NS_ERROR_UNEXPECTED; >+ } >+ *webProgress = mChildProcess; >+ NS_ADDREF(*webProgress); >+ return NS_OK; >+ } >+#endif >+ >+ nsCOMPtr<nsIDocShell> shell; >+ rv = GetDocShell(getter_AddRefs(shell)); >+ if (NS_SUCCEEDED(rv)) { >+ nsCOMPtr<nsIWebProgress> progress(do_QueryInterface(shell)); >+ NS_ADDREF(*webProgress = progress); progress.swap(*aWebProgress); >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >@@ -3987,17 +3987,16 @@ nsDocShell::Reload(PRUint32 aReloadFlags > nsnull, // No headers data > loadType, // Load type > nsnull, // No SHEntry > PR_TRUE, > nsnull, // No nsIDocShell > nsnull); // No nsIRequest > } > >- This is some random change. Could you not do it. >+// Only one of OnProgressChange / OnProgressChange64 will be called. >+// According to interface, it should be OnProgressChange64, but looks >+// like docLoader only sends the former. Worth to file a followup bug? >+NS_IMETHODIMP >+TabChild::OnLocationChange(nsIWebProgress *aWebProgress, >+ nsIRequest *aRequest, >+ nsIURI *aLocation) >+{ >+ nsCString uri; >+ aLocation->GetSpec(uri); Null check aLocation >+TabChild::OnRefreshAttempted(nsIWebProgress *aWebProgress, >+ nsIURI *aURI, PRInt32 aMillis, >+ PRBool aSameURL, PRBool *aRefreshAllowed) >+{ >+ nsCString uri; >+ aURI->GetSpec(uri); Null check aURI >+void >+TabParent::CheckCompactListeners() >+{ >+ if (mUncompactedElementsRemoved > COMPACT_LIMIT) { >+ mUncompactedElementsRemoved = 0; >+ mListenerInfoList.Compact(); If you change this a bit, you don't need COMPACT_LIMIT. Change the code a bit so that you remove the weakptr which doesn't point to anywhere here, and then if something is removed, call compact. listeners shouldn't be going away too often. >+ } >+} >+ >+bool >+TabParent::RecvnotifyProgressChange(const PRInt64& aProgress, >+ const PRInt64& aProgressMax, >+ const PRInt64& aTotalProgress, >+ const PRInt64& aMaxTotalProgress) Fix indentation. >+{ >+ /* >+ * First notify any listeners of the new progress info... >+ * >+ * Operate the elements from back to front so that if items get >+ * get removed from the list it won't affect our iteration >+ */ >+ nsCOMPtr<nsIWebProgressListener> listener; >+ PRInt32 count = mListenerInfoList.Length(); >+ >+ while (--count >= 0) { >+ TabParentListenerInfo *info = &mListenerInfoList[count]; >+ if (!(info->mNotifyMask & nsIWebProgress::NOTIFY_PROGRESS)) { >+ continue; >+ } >+ >+ listener = do_QueryReferent(info->mWeakListener); >+ if (!listener) { >+ // the listener went away. gracefully pull it out of the list. >+ mListenerInfoList.RemoveElementAt(count); >+ mUncompactedElementsRemoved++; >+ CheckCompactListeners(); >+ continue; You have this code copy-pasted all over. Include in CheckCompactListeners. And rename that method, maybe to RemoveListenerAndCompact(PRUint32 aIndex); And nit, mUncompactedElementsRemoved++ is slower than ++mUncompactedElementsRemoved >+ } >+ >+ nsCOMPtr<nsIWebProgressListener2> listener2 >+ = do_QueryReferent(info->mWeakListener); Use 2 space indentation. Same problem also in the next few lines. And elsewhere. >+NS_IMETHODIMP >+TabParent::RemoveProgressListener(nsIWebProgressListener *aListener) >+{ >+ nsAutoPtr<TabParentListenerInfo> info(GetListenerInfo(aListener)); >+ >+ return info && mListenerInfoList.RemoveElement(*info) >+ ? NS_OK : NS_ERROR_FAILURE; Nit, ? should be in the previous line >+} >+ >+TabParentListenerInfo * >+TabParent::GetListenerInfo(nsIWebProgressListener *aListener) >+{ >+ PRInt32 i, count; >+ TabParentListenerInfo *info; >+ >+ nsCOMPtr<nsISupports> listener1 = do_QueryInterface(aListener); >+ count = mListenerInfoList.Length(); >+ for (i = 0; i < count; ++i) { >+ info = &mListenerInfoList[i]; >+ >+ NS_ASSERTION(info, "There should NEVER be a null listener in the list"); >+ if (info) { >+ nsCOMPtr<nsISupports> listener2 = do_QueryReferent(info->mWeakListener); >+ if (listener1 == listener2) >+ return info; if (listener1 == listener2) { return info; } When iterating mListenerInfoList, use PRUint32. >+// The flags passed by the webProgress notifications are 16 bits shifted >+// from the ones registed by webProgressListeners. >+#define NOTIFY_FLAG_SHIFT 16 I don't understand this. In the client process TabChild is a webprogresslistener, so it gets aStateFlags parameter. What we need to shift that?

>>+// Only one of OnProgressChange / OnProgressChange64 will be called. >>+// According to interface, it should be OnProgressChange64, but looks >>+// like docLoader only sends the former. >Worth to file a followup bug? Yes, I'll file and take this. bug 554144 I decided to take out the Compact() calls. Seems that if we don't know the add/remove pattern of listeners, it probably makes sense to let nsTArray take care of its own storage. > When iterating mListenerInfoList, use PRUint32. Originally this was >PRInt32 count = array.Length(); >while (--count >= 0) { ... } Changed to: >PRUint32 count = array.Length(); >while (count-- > 0) { ... } which seems worse than just using a signed integer. Let me know if you think the decrement should go on the next line (still seems unfortunate), or if I should just stick to signed ints. The backwards iteration is nice because I'm removing some elements from the array as I iterate. >In the client process TabChild is a webprogresslistener, so it gets aStateFlags >parameter. >What we need to shift that? nsIWebProgress allows listeners to specify certain kinds of state that they want to receive when they register themselves. So, we send only the notifications that are relevant to that listener's interests. It happens that the flags used to "register" for a certain type of notification are exactly 16 bits shifted from the flags used to indicate that a notification is of a certain type. Flags used during registration are here: http://mxr.mozilla.org/projects-central/source/electrolysis/uriloader/base/nsIWebProgress.idl#98 Flags sent with notifications are here: http://mxr.mozilla.org/projects-central/source/electrolysis/uriloader/base/nsIWebProgressListener.idl#162 Fixed other comments.

Comment on attachment 434034 [details] [diff] [review] addresses review comments I just realized, I guess I need a superreview for the idl change? If this isn't true, please feel free to cancel. I added this attribute nsFrameLoader so that we can get a reference to the TabParent (which implements webProgress) in browser.xml.

Clarification: in non-remote browsers, the webProgress attribute on a frameloader just points to the docshell.

(In reply to comment #32) > nsIWebProgress allows listeners to specify certain kinds of state that they > want to receive when they register themselves. So, we send only the > notifications that are relevant to that listener's interests. It happens that > the flags used to "register" for a certain type of notification are exactly 16 > bits shifted from the flags used to indicate that a notification is of a > certain type. > > Flags used during registration are here: > http://mxr.mozilla.org/projects-central/source/electrolysis/uriloader/base/nsIWebProgress.idl#98 > > Flags sent with notifications are here: > http://mxr.mozilla.org/projects-central/source/electrolysis/uriloader/base/nsIWebProgressListener.idl#162 But the state is already the state for and WebProgressListener, the listener in content process. Shouldn't the possible bit shift have happened already when content process webprogresslistener is called?

The bitshift is done only for the purpose of comparing it to the listener's registration flag. So the notification is still passed along with the unshifted representation, as required by the interface specification. My code passes the notification from content to chrome to listeners in chrome, but doesn't change the value of the flags that are passed. It's true that two bitshifts are being done: one in the content process, in nsDocLoader's code (where it compares the notification's state flags to TabChild's registration flags), and one in the chrome process, in TabParent's code (where it compares the notifications state flags to the registration flags of TabParent's listeners). In both cases, the bitshifted representation is thrown away with the stack, and the unshifted representation is passed along.

(In reply to comment #36) > It's true that two > bitshifts are being done: one in the content process, in nsDocLoader's code > (where it compares the notification's state flags to TabChild's registration > flags), and one in the chrome process, in TabParent's code (where it compares > the notifications state flags to the registration flags of TabParent's > listeners). Ok. Since this all is quite ugly, could you please add some comment and link to the nsDocLoader near the place where you do bit shift.

Comment on attachment 434034 [details] [diff] [review] addresses review comments >+++ b/content/base/test/chrome/bug514705_helper.xul >@@ -0,0 +1,90 @@ >+<?xml version="1.0"?> >+<?xml-stylesheet href="chrome://global/skin" type="text/css"?> >+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?> >+ >+<window title="Bug514705 helper" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ onload="RunTest();"> >+ >+ <script type="application/javascript"> >+ <![CDATA[ >+ var Ci = Components.interfaces; >+ var imports = [ "SimpleTest", "is", "isnot", "ok" ]; >+ for each (var import in imports) { >+ window[import] = window.opener.wrappedJSObject[import]; >+ } >+ var locationChanged = false; >+ var progressChanged = false; >+ var refreshAttempted = false; >+ >+ var listener = { >+ onLocationChange: function(webProgress, request, location) { >+ locationChanged = true; >+ }, >+ onProgressChange: function(webProgress, request, curSelfProgress, >+ maxSelfProgress, curTotalProgress, >+ maxTotalProgress) { >+ }, >+ onSecurityChange: function(webProgress, request, state) { >+ }, >+ onStateChange: function(webProgress, request, stateFlags, status) { >+ >+ if ((stateFlags & Ci.nsIWebProgressListener.STATE_STOP) >+ && (stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW)) { '&&' should be in the previous line. >@@ -116,29 +118,33 @@ > > nsCOMPtr<nsIWebBrowser> webBrowser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID); > if (!webBrowser) { > NS_ERROR("Couldn't create a nsWebBrowser?"); > return NS_ERROR_FAILURE; > } > > webBrowser->SetContainerWindow(this); >+ nsCOMPtr<nsIWeakReference> weak >+ = do_GetWeakReference(static_cast<nsSupportsWeakReference*>(this)); Nit, '=' should be in the previous line. >+// Only one of OnProgressChange / OnProgressChange64 will be called. >+// According to interface, it should be OnProgressChange64, but looks >+// like docLoader only sends the former. Ok, seems that currently OnProgressChange64 is for downloading. But since it is easy to support here, let's keep it. >+TabParent::RecvnotifyProgressChange(const PRInt64& aProgress, >+ const PRInt64& aProgressMax, >+ const PRInt64& aTotalProgress, >+ const PRInt64& aMaxTotalProgress) ... >+ nsCOMPtr<nsIWebProgressListener2> listener2 >+ = do_QueryReferent(info->mWeakListener); '=' should be in the previous line. >+TabParent::RecvrefreshAttempted(const nsCString& aURI, const PRInt32& aMillis, >+ const bool& aSameURI, bool* refreshAllowed) ... >+ nsCOMPtr<nsIWebProgressListener2> listener2 >+ = do_QueryReferent(info->mWeakListener); Nit, '=' should be in the previous line. >+TabParentListenerInfo * >+TabParent::GetListenerInfo(nsIWebProgressListener *aListener) >+{ >+ PRUint32 i, count; >+ TabParentListenerInfo *info; >+ >+ nsCOMPtr<nsISupports> listener1 = do_QueryInterface(aListener); >+ count = mListenerInfoList.Length(); >+ for (i = 0; i < count; ++i) { >+ info = &mListenerInfoList[i]; >+ >+ NS_ASSERTION(info, "There should NEVER be a null listener in the list"); >+ if (info) { Useless assertion, since you handle the null case anyway. Either remove the assertion and the null check, or replace assertion with an NS_WARN_IF_FALSE(). I prefer the first option. >-class TabParent : public PIFrameEmbeddingParent >+struct TabParentListenerInfo >+{ >+ TabParentListenerInfo(nsIWeakReference *aListener, unsigned long aNotifyMask) >+ : mWeakListener(aListener), mNotifyMask(aNotifyMask) >+ { >+ } >+ >+ TabParentListenerInfo(const TabParentListenerInfo& obj) >+ : mWeakListener(obj.mWeakListener), mNotifyMask(obj.mNotifyMask) >+ { >+ } >+ >+ nsWeakPtr mWeakListener; >+ >+ unsigned long mNotifyMask; PRUint32?

Comment on attachment 434034 [details] [diff] [review] addresses review comments API review looks fine

I will need someone to check this in for me, but waiting for some build fixes to land so I can use the TryServer. Will mark checkin-needed (or new patch) when ready!

Comment on attachment 434720 [details] [diff] [review] fixes above comments, unbitrotted http://hg.mozilla.org/projects/electrolysis/rev/cd86e0ea3d2d

Tests are failing on tbox.

That could be also because of the merge from m-c. The failure is actually a crash.

The failure seems to happen because of some bug in crashreporter. Some bugs have been fixed on trunk, but I'm not sure which one might fix the crash on e10s.

I disabled the test for know http://hg.mozilla.org/projects/electrolysis/rev/55459d314fff Need to figure out why crashreporter code crashes in child process. It doesn't seem to have anything to do with the testcase.

olli, is this still a problem.

uh, the test might be. I need to look at that. (and the problem was really crashreporter)

That's really a question for mfinkle now, right?: we want <browser>.webProgress to work, basically. We can either use custom protocol stuff, or the message manager, as long as it works in the end.

The current WIP for a remote-aware <browser> binding adds .webProgress support using messageManager for the implementation See (it's in there with other stuff): http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/rev/5871bc5f11bf Since this patch landed, we can back out the .webProgress support in the <browser>, right?

olli, do we want a separate bug for the tests?

We started to remove the JS, messageManager-based web progress implementation from the browser binding, but I hit a snag. Many times, the chrome (UI) process only wants to pay attention to the web progress listener methods if the activity is for the root document, not a frame. You would see code like this: if (aWebProgress.DOMWindow == someBrowser.contentDocument) { // do something } But we can't do this with the e10s implmentation. We can do it with the JS, messageManager-based solution because we add an "isRootDocument" property to the JSON sent back. Any ideas for how we could tweak the IPC version of web progress listeners to handle this?

Maybe one possibility is to create some simple object on the chrome side that implements webProgress and contains information like isRootDocument and pass that to listeners instead of passing the TabParent itself. We'd need to extend the webProgress interface to have the isRootDocument attribute. Then, it should be easy to modify the IPDL to pass that information along (just do the aWebProgress.DOMWindow == aWebProgress.DOMWindow.parent test on the content side). I'm not sure I completely understand the message-manager solution above -- it looks like it implements some different interface that sends notifications with paramaters of 'isRootWindow', 'browser', and sometimes 'identity'. Is that right? Is there

This patch takes the approach I outlined above, so a webProgressListener can do: >webProgress.QueryInterface(Components.interfaces.nsIWebProgress2).isRootDocument() to discover whether the notification fired from a root document. Does this seem like a reasonable solution?

We have been toying with various ideas about how to pass back an window/frame identifier from WebProgress and other messages. Ideas included: * isRootDocument: very specific to a use case in Fennec front-end, but not very helpful in general. * documentURI: general purpose, but dealing with a string URI can be a pain, and there are timing issues about when the URI would be valid, and sub frames could have the same URI. * windowID: Boris told me about window IDs and how we could use those to uniquely identify a window without URIs and lifetime issues. I like the windowID approach and would suggest adding a DOMWindowID property to the TabParent/WebProgress implementation. It nicely mirrors the current DOMWindow property in non-e10s. We will also add a .contentWindowID property to the browser XBL binding. This will allow users to _almost_ mimic what happens in non-e10s, where webProgress.DOMWindow is compared to browser.contentWindow Thoughts?

Allows a webProgressListener (of a remote browser) to use: >webProgress.QueryInterface(Components.interfaces.nsIDOMWindowIDHolder).DOMWindowID to get the DOMWindowID of the window that fired the notification. Need to figure out how to fix the test (comment 42) so I can update it to cover this case. Olli, any ideas? I asked jmaher for some help on the test also, so hopefully I will figure that out soon.

Attaches a listener to the remote browser in test.xul, and reports the DOMWindowID on location change. Also: do I need to add this attribute for webProgress of non-remote browsers as well?

(In reply to comment #56) > Also: do I need to add this attribute for webProgress of non-remote browsers as > well? Yes. We have front-end code that uses the same webprogresslistener code for both remote and local browsers.

I should have asked this before, but just to confirm: the window IDs that you're talking about are specifically the mWindowIDs of nsGlobalWindows, right (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.h#834)? In a couple of different places in the code, I have assumed / will assume that the nsIDOMWindow returned by nsDocShell's GetDomWindow is actually a nsGlobalWindow, ie that I can QueryReference it to some other interface that nsGlobalWindow implements.

> I should have asked this before, but just to confirm: the window IDs that > you're talking about are specifically the mWindowIDs of nsGlobalWindows, Yes. > I have assumed / will assume that the nsIDOMWindow returned by nsDocShell's > GetDomWindow is actually a nsGlobalWindow It will be a subclass of nsGlobalWindow, yes.

Implemented this nsIDOMWindowIDHolder for non-remote browser's webprogress as well. The test for this one actually works!

We need to be careful: the window IDs are guaranteed to be unique per-process, but we should plan on this working with process per-tab, so you might need to combine a process-ID with a window-ID to get something actually unique.

About getting a process-ID, bsmedberg and I think that putting a static counter in TabParent is preferable to using OS pID because OS pID can be reused. With that in mind, started thinking about this part of mfinkle's comment: >We will also add a .contentWindowID property to the browser XBL binding. This >will allow users to _almost_ mimic what happens in non-e10s, where >webProgress.DOMWindow is compared to browser.contentWindow I wasn't sure if this property is going to be added as part of this bug (seems like it might have to be?) Also seems kind of tricky, because it needs to get this pID from the TabParent and also the windowID from the nsGlobalWindow running in the content process, so I'm not sure how to do the second part. I guess the property would be stored in nsContainerBoxObject? The windowID would need to be sent from content to chrome during creation of a new root document(?), since chrome can't block on asking content what it is.

Currently, in the browser.xml binding in mobile-browser, we send the windowID back from content to chrome in onLocationChange and the borwser binding caches it in a browser.contentWindowId property. I think we will change to DOMWindowCreated, unless some other means becomes available through this patch.

Implements nsIDOMWindowIDHolder for all webprogress objects, as described above. Implements contentWindowID for browser objects, cribbing the locationChanged approach from mobile code (but it handles the ID in c++ code, instead of in the browser binding, because it notifies the other listeners in c++ as well. Handling it in browser binding might be too late here, if other listeners are notified first.) Enables test for both non-remote and remote browser, although the test for remote won't work until the crashreporter crash is fixed (bug 575729).

...allowing the remote browser to work in mochitests without crashing in child process. With this patch applied, the tests included in the above patch should run. I also have a patch to fix some historyService NPE in my queue, but I think that one already has a patch landing soon.

fixes a problem with appending PIDs, everything else as above.

Comment on attachment 455505 [details] [diff] [review] adds nsIDOMWindowIDHolder with attribute DOMWindowID, plus tests > NS_IMETHODIMP > TabChild::OnStateChange(nsIWebProgress *aWebProgress, >- SendnotifyStateChange(aStateFlags, aStatus); >+ nsCString str; >+ nsresult rv = GetWindowID(aWebProgress, str); You might want to use a better variable name in these methods. Even "idStr" is better. This patch looks good. The DOMWindowId property is added to webProgress for both IPC and non-IPC and the browser.xml binding change will mean anyone can use the property at any time. The only part that has me worried is that the DOMWindowId is not setup until OnLocationChange. Fortunately for Fennec, this is the same time we setup the contentWindowId, so it should work fine for now. You should file a new bug to start using the DOMWindowCreated event, or the corresponding observer notification - whichever is easier. r+ from me, but you definitely need to get someone else to review this too.

Asking smaug for review, per mfinkle's comment above.

Comment on attachment 460390 [details] [diff] [review] unbitrotted, addresses above review comment. Does this cause us to expose window.DOMWindowID to the webcontent? We don't want that. mozWindowID might be possible. And why you need to handle the WindowID as string? Wouldn't unsigned long long in the idl be ok?

And could we stick the MozWindowID (or whatever to call it) to some existing interface? nsGlobalWindow implements plenty of interfaces already. Adding a new interface just for this one new property would be unfortunate.

(In reply to comment #69) > And why you need to handle the WindowID as string? > Wouldn't unsigned long long in the idl be ok? I think we need the process pid and the window id, concatenated, to provide a true unique ID, when we move to multiple content processes.

(In reply to comment #70) > And could we stick the MozWindowID (or whatever to call it) to some existing > interface? nsGlobalWindow implements plenty of interfaces already. > Adding a new interface just for this one new property would be unfortunate. We only need this exposed to chrome for our purposes. I don't think content needs access at all. Maybe we could add this to nsIDOMWindowUtils?

I was asking whether content *can* access it. I agree, we may need some process+windowID, but the interface adds just WindowID. pid + windowid isn't guaranteed to be unique, I think, if it is generated in a content process. If we kill a child process, a new child process may get the same pid, right? Should chrome process give content process an ID?

Yeah, this thing should go on nsIDOMWindowUtils. Note that we already have window ids on there, perhaps those should just be changed to strings?

I can change nsGlobalWindow to use nsIDOMWindowUtils with an added property, but I'm not so sure about having the webProgress classes (TabParent and nsDocLoader) implement nsIDOMWindowUtils. I should add some comments to the interface, but basically the promise is that the WindowID given by nsIDOMWindowIDHolder is unique within the process. So in a child process, just a windowID is sufficient (and that's all you get). In the parent process, you get a pID+windowID, where pID is basically just a counter on TabParent (so each new TabParent gets a new pID). So this solves the question of what happens if a child process is killed and another one is created. Open to suggestions on new names for the interface or property too, if windowID doesn't convey the desired meaning. Does removing the nsIDOMWindowIDHolder interface from nsGlobalWindow and adding a new property to nsIDOMWindowUtils solve the problem of accidentally exposing this to content?

> but I'm not so sure about having the webProgress classes (TabParent and > nsDocLoader) implement nsIDOMWindowUtils Why would they need to? Why are we putting anything like this on nsDocLoader anyway? We can use a new interface, but it needs to be implemented by a non-web-exposed object (e.g. the window utils object). > I should add some comments to the interface, but basically the promise is that > the WindowID given by nsIDOMWindowIDHolder is unique within the process Uh.... Then why do we need it at all? Existing window ids give us that.... We need an interface on tabparent to handle this, but you're saying it would be a different interface anyway, right? > Does removing the nsIDOMWindowIDHolder interface from nsGlobalWindow and > adding a new property to nsIDOMWindowUtils solve the problem of accidentally > exposing this to content? Yes. It's one possible solution.

The usage we had in mind is that a client can do a comparison like browser.webProgress.QueryInterface(nsIDOMWindowIDHolder).DOMWindowID == webProgress.QueryInterface(nsIDOMWindowIDHolder).DOMWindowID where the second webProgress object is the one that is passed back to the webProgressListener in OnStateChanged() events. So the above equality would hold if the event came from the root window, as opposed to an iframe. mfinkle tells me there is code that doesn't distinguish between remote and non-remote browsers, so (I think) the exposing interface needs to be the same for all webProgress objects. Currently, that's TabParent and nsDocLoader.

Make it GetInterface instead of QI, and you don't have to add random crud to docloader other than changing docshell's (NOT docloader's, which may not even have a window on it) GetInterface implementation, right?

So after there has been so much work on this bug, I hate to squash it, but Fennec has already implemented the parts of this bug that they need using the message manager, and are talking about implementing content-side filtering or policy to the webprogress messages anyway. Can we just leave that as a permanent solution, and WONTFIX this bug?

Maybe we can still use the tests! Definitely would be nice to know whether this path is still useful/desired before putting more development into it, though. (mfinkle, any thoughts?) The next step (from IRC discussion) would be to take advantage of the unfrozen webProgressListener interface and add a windowID parameter to each of the events, so we wouldn't need any of these additional interfaces.

I would hate to squash this bug too. The messageManager version of webprogress listeners _does_ now support filtering some message types (onProgress and onStatus) on the child side, so we don't send unneeded messages. That type of support would be needed here too, eventually. (In reply to comment #77) > The usage we had in mind is that a client can do a comparison like > > browser.webProgress.QueryInterface(nsIDOMWindowIDHolder).DOMWindowID == > webProgress.QueryInterface(nsIDOMWindowIDHolder).DOMWindowID > > where the second webProgress object is the one that is passed back to the > webProgressListener in OnStateChanged() events. So the above equality would > hold if the event came from the root window, as opposed to an iframe. The usage is: browser.contentWindowId == webprogress.DOMWindowId but the usecase is the same > mfinkle tells me there is code that doesn't distinguish between remote and > non-remote browsers, so (I think) the exposing interface needs to be the same > for all webProgress objects. Currently, that's TabParent and nsDocLoader. Also true. We do not want front-end code to ever need to ask "is this coming from a child process". The code should work transparently. In the messageManager code, we use the message-based webprogress listener system for both in-process and out-of-process content windows. It's not as seemless as this patch would be. (In reply to comment #79) > So after there has been so much work on this bug, I hate to squash it, but > Fennec has already implemented the parts of this bug that they need using the > message manager, and are talking about implementing content-side filtering or > policy to the webprogress messages anyway. Can we just leave that as a > permanent solution, and WONTFIX this bug? That would be OK for now. We might run into issues later where the messageManager based system is not good enough and we can revisit this patch.

Executive decision: we're going to WONTFIX this, gradually deprecate <browser>.webProgress in favor of webprogress-via-messagemanager. Please file bugs if there are ways to make it *easier* to use webprogress via the message manager.

(In reply to comment #82) > Executive decision: we're going to WONTFIX this, gradually deprecate > <browser>.webProgress in favor of webprogress-via-messagemanager. Please file > bugs if there are ways to make it *easier* to use webprogress via the message > manager. Just for those interested: Fennec implements <browser>.webProgress using webprogress-via-messagemanager. For the messy details, see: http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.xml#159 && 483 (yes, some duplication currently exists) http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings/browser.js#6