292971 - Sort out the progress listener story with fastback/bfcache

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We need to sort out what happens with progress listeners on fastback/bfcache. 
Having everyone who currently listens to progress listeners for page loads go
through the contortions that the Firefox password manager goes through in the
patch from bug 274784 is not cool.  Right now all you have to do is get a
service or reference to a docshell and register yourself with it to watch all
document loads (globally and in that docshell respectively); whatever we do
should be roughly equivalent in complexity for the consumer.

I still think the simplest solution is to push the mChannel on documents up to
nsDocument (from HTML/XMLDocument) and call OnStateChange with this channel as
the request.  A minor tweak to docshell to prevent onload firing when we do this
should be pretty straightforward.

If we decide not to do this, then we should either add some notifications to
nsIWebProgressListener2 (preferred) or create a new interface for this...

Comment 1

[Darin Fisher]

> I still think the simplest solution is to push the mChannel on documents up to
> nsDocument (from HTML/XMLDocument) and call OnStateChange with this channel as
> the request.  A minor tweak to docshell to prevent onload firing when we do this
> should be pretty straightforward.

I second this proposal.  It sounds reasonable, simple, and relatively low risk.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Push the mChannel up to nsDocument, expose on nsIDocument

jst, what do you think?

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

Yeah, fine with me. r+sr=jst

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

Requesting 1.8b3.  This should be pretty safe, and we need something like this
if we want fastback delivering progress listener notifications.

Comment 5

[chris hofmann]

Comment on attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

a=chofmann,  boris, did you mean 1.8b2?

Comment 6

[Boris Zbarsky [:bzbarsky]]

No, I meant 1.8b3.  If we're shipping 1.8b2 any time soon (which I hope we are,
but that was the story when I requested the approval too), I won't have time to
fix possible regressions from this before then.

If someone wants to check this in for 1.8b2, feel free to, of course.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

Checked in this change.

Comment 8

[Brian Ryner (not reading)]

Attachment: patch

This patch actually addresses a number of issues.  Much of this is detailed at
http://wiki.mozilla.org/User:Bryner

First off, it enables nsIWebProgressListener notifications for fastback.  The
document's original channel is used as the request.  A new bit,
STATE_IS_RESTORE, is added so that implementations can respond differently to
non-original-load notifications.

Secondly, it drops the DOMPageRestore event in favor of two new events,
PageShow and PageHide.	These are discussed in-depth on the wiki page.	Event
listeners in the chrome which actually want show/hide notification instead of
load notification are switched over.  Load and unload events are no longer
dispatched at all for fastback loads.

Fixed a problem where the window's event listeners get cleared out via
nsGlobalWindow::SetNewDocument, after saving the event listener state.	I fixed
this by removing the EventListenerManager from the window when it's saved into
a WindowStateHolder.  Also made sure to save and restore mMutationBits.

Changed the placement of calls to CaptureState() so that we don't capture state
until all event listeners for the unload have run.  This is important in case a
listener modifies the window object.

Reverted the changes to the PasswordManager and SecureBrowserUI classes that
were needed to account for lack of WebProgressListener notifications.

There is one minor inconsistency I note now, but I'm not sure it's necessarily
a bug in this patch.  The DOMLinkRemoved event (which has no consumers in our
code) is fired during regular unload, but because the root element has already
removed itself from the document, the event does not bubble up to the document
or window.  I fire this event from nsDocument::OnPageHide() in this patch, but
the event does bubble all the way to the window in this case.  It seems to me
like the current behavior is "odd" and we probably need to figure out what the
right way is for this event to behave.

Comment 9

[Darin Fisher]

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

>Index: browser/base/content/browser.js

>   // update the last visited date
>-  if (targetBrowser.currentURI.spec)
>+  if (!event.persisted && targetBrowser.currentURI.spec)
>     BMSVC.updateLastVisitedDate(targetBrowser.currentURI.spec,
>                                 targetBrowser.contentDocument.characterSet);
> }

From the point of view of the user, aren't they visiting a page
when they press the back button to view it?  I would think that
we'd want to update the last-visited timestamp stored in bookmarks
when restoring a page from the bfcache.


>Index: content/base/public/nsIDocument.h

>+  /**
>+   * Notification that the page has been shown.  This corresponds to the
>+   * completion of document load, or to the page's presentation being restored
>+   * into an existing DOM window.  This notification fires applicable DOM
>+   * events to the content window.  See nsIDOMPageChangeEvent.idl for a
>+   * description of the |aPersisted| parameter.
>+   */
>+  virtual void OnPageShow(PRBool aPersisted) = 0;
>+
>+  /**
>+   * Notification that the page has been hidden.  This corresponds to the
>+   * unloading of the document, or to the document's presentation being saved
>+   * but removed from an existing DOM window.  This notification fires
>+   * applicable DOM events to the content window.  See
>+   * nsIDOMPageChangeEvent.idl for a description of the |aPersisted| parameter.
>+   */
>+  virtual void OnPageHide(PRBool aPersisted) = 0;

I think you should change the UUID of nsIDocument.


>Index: content/events/src/nsDOMPageChangeEvent.cpp

>+nsDOMPageChangeEvent::nsDOMPageChangeEvent(nsPresContext* aPresContext,
>+                                           nsPageChangeEvent* aEvent)
>+  : nsDOMEvent(aPresContext, aEvent ? aEvent :
>+               new nsPageChangeEvent(PR_FALSE, 0, PR_FALSE))

what if new fails?


>Index: docshell/base/nsDocShell.cpp

>+class RestorePresentationEvent : public PLEvent
>+{
>+public:
>+    RestorePresentationEvent(nsDocShell *aShell)
>+        : mDocShell(aShell)
>+    {
>+    }

I like to call PL_InitEvent from the constructor of a class
that subclasses PLEvent.  It often results in nicer code.


>+nsDocShell::BeginRestore(PRBool aTop)
...
>+    if (aTop) {
>+        // We finish up the restore processing on a PLEvent so that we don't
>+        // grow the stack too deep in RemoveRequest().

"... to mimic the way RemoveRequest is called by nsIChannel implementations."


>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mContentViewer->GetDOMDocument(getter_AddRefs(domDoc));
> 
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
>+    if (doc) {
>+        // Finally, we remove the request from the loadgroup.  This will cause
>+        // onStateChange(STATE_STOP) to fire, which will fire the onload event

onload or PageShow?


>Index: docshell/base/nsIContentViewer.idl

>   void loadStart(in nsISupports aDoc);
>   void loadComplete(in unsigned long aStatus);
>   boolean permitUnload();
>-  void unload();
>+  void pageHide(in boolean isUnload);
> 

Change a UUID?


>Index: dom/public/coreEvents/nsIDOMPageChangeListener.h

>+/*
>+ * Page change event listener interface.
>+ */
>+#define NS_IDOMPAGECHANGELISTENER_IID \
>+{ 0x24f4d69f, 0x6b0c, 0x48a8, { 0xba, 0xb7, 0x12, 0x50, 0xcb, 0x5e, 0x48, 0x79 } }
>+
>+class nsIDOMPageChangeListener : public nsIDOMEventListener {
>+ public:
>+  NS_DEFINE_STATIC_IID_ACCESSOR(NS_IDOMPAGECHANGELISTENER_IID)

Any reason not to use XPIDL for this?


>Index: dom/src/base/nsGlobalWindow.cpp

>   aWindow->GetListenerManager(getter_AddRefs(mListenerManager));
>+  mMutationBits = aWindow->mMutationBits;
>+
>+  // Clear the window's EventListenerManager pointer so that it can't have
>+  // listeners removed from it later.
>+  aWindow->mListenerManager = nsnull;
...
>+  mMutationBits = holder->GetMutationBits();

I don't really understand the mutation bits stuff.  Be sure to get a
review from someone who does :)


>Index: uriloader/base/nsIWebProgressListener.idl

>   const unsigned long STATE_IS_REQUEST     = 0x00010000;
>   const unsigned long STATE_IS_DOCUMENT    = 0x00020000;
>   const unsigned long STATE_IS_NETWORK     = 0x00040000;
>   const unsigned long STATE_IS_WINDOW      = 0x00080000;
>+  const unsigned long STATE_IS_RESTORE     = 0x00100000;

One thing confusing about this is that the other 4 things
are objects, whereas this is more like an action.  I guess
that's ok.

Notice that nsIWebProgress specifies NOTIFY_STATE_ flags
that can be used to limit the type of objects a listener
may observe.  We aren't adding to that list here, and that
seems a little odd.

This is in part why I originally suggested augmenting the
other set of STATE_ flags (making this STATE_RESTORING).
However, the documentation for those says that they are
mutually exclusive :(

Maybe we need a new set of STATE_ flags that are modifiers.
Maybe the modifiers should occupy the high byte in the state
flags.


>Index: xpfe/browser/resources/content/navigator.js

>-    UpdateBookmarksLastVisitedDate(event);
>+    if (!event.persisted) {
>+      UpdateBookmarksLastVisitedDate(event);
>+    }

same question that i had about browser.js above.

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #9)
> From the point of view of the user, aren't they visiting a page
> when they press the back button to view it?  I would think that
> we'd want to update the last-visited timestamp stored in bookmarks
> when restoring a page from the bfcache.

That's true, though they aren't visiting a version of the page that's known to
be current.  I guess that's the case now as well.

> >Index: content/events/src/nsDOMPageChangeEvent.cpp
> 
> >+nsDOMPageChangeEvent::nsDOMPageChangeEvent(nsPresContext* aPresContext,
> >+                                           nsPageChangeEvent* aEvent)
> >+  : nsDOMEvent(aPresContext, aEvent ? aEvent :
> >+               new nsPageChangeEvent(PR_FALSE, 0, PR_FALSE))
> 
> what if new fails?

I'm just mimicing the way the other DOM event types work.  I don't think that
needs to be rearchitected as part of this change.

> >Index: dom/public/coreEvents/nsIDOMPageChangeListener.h
> 
> >+/*
> >+ * Page change event listener interface.
> >+ */
> >+#define NS_IDOMPAGECHANGELISTENER_IID \
> >+{ 0x24f4d69f, 0x6b0c, 0x48a8, { 0xba, 0xb7, 0x12, 0x50, 0xcb, 0x5e, 0x48,
0x79 } }
> >+
> >+class nsIDOMPageChangeListener : public nsIDOMEventListener {
> >+ public:
> >+  NS_DEFINE_STATIC_IID_ACCESSOR(NS_IDOMPAGECHANGELISTENER_IID)
> 
> Any reason not to use XPIDL for this?
> 

It's the same as the way the other event listener interfaces work.  I think they
_should_ use IDL, but I think that's outside the scope of this change.

> 
> >Index: dom/src/base/nsGlobalWindow.cpp
> 
> >   aWindow->GetListenerManager(getter_AddRefs(mListenerManager));
> >+  mMutationBits = aWindow->mMutationBits;
> >+
> >+  // Clear the window's EventListenerManager pointer so that it can't have
> >+  // listeners removed from it later.
> >+  aWindow->mListenerManager = nsnull;
> ...
> >+  mMutationBits = holder->GetMutationBits();
> 
> I don't really understand the mutation bits stuff.  Be sure to get a
> review from someone who does :)
> 

It's just a few bits on the window object that say what sorts of mutation
listeners exist somewhere in the window.  It's used to short-circuit firing
mutation events if no one is listening for them.  We need to restore that state
on fastback since elements in the window may now have mutation listeners.

Comment 11

[Brian Ryner (not reading)]

Attachment: patch w/ darin's comments addressed

Addressed darin's comments except as noted above

Comment 12

[Darin Fisher]

Comment on attachment 186163 [details] [diff] [review]
patch w/ darin's comments addressed

We need to worry about things like DocShell.loadURI being called between
BeginRestore and FinishRestore.

Comment 13

[Brian Ryner (not reading)]

Attachment: revoke pending events on Stop()

This should address that problem.  Also, I changed the sr request to sicking to
balance the load while Boris is out of town. (Boris, your comments are of
course welcome)

Comment 14

[Darin Fisher]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

>Index: docshell/base/nsDocShell.cpp

>     if (nsIWebNavigation::STOP_CONTENT & aStopFlags) {
>         if (mContentViewer)
>             mContentViewer->Stop();
>+
>+        // Revoke any pending plevents related to content viewer restoration
>+        nsCOMPtr<nsIEventQueue> uiThreadQueue;
>+        NS_GetMainEventQ(getter_AddRefs(uiThreadQueue));
>+        uiThreadQueue->RevokeEvents(this);
>     }

Do we have to worry at all about side-effects from mContentViewer->Stop() ?

Should we RevokeEvents before calling Stop() ?

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

I made that change locally (revoke events before contentviewer->Stop())

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

Sorry, I'm not a superreviewer.

Comment 17

[Darin Fisher]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

Jonas: pretend that I gave SR :-)

Comment 18

[Brian Ryner (not reading)]

Jonas: Yes, I'm more interested in you or Boris looking at the code than I am
the formalities of our review process.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

I didn't have a chance to read the code in detail, so what follows are just
general comments.

I'm really happy that the presentation restoration is now effectively async
(and that we cancel it on Stop() being called).  That should make the model
much simpler to work with, since all loads wll be async again.	It's also very
nice to be reusing the existing codepaths more!

>Index: content/base/public/nsIDocument.h

>+   * Notification that the page has been shown.  This corresponds to the
>+   * completion of document load, or to the page's presentation being restored
>+   * into an existing DOM window.

Does this happen for documents loaded as data (so without a window)?  That's
not clear from the description, which talks about DOM windows....   It looks
like it doesn't, since this is all handled docshell/viewer code, and that may
be worth clarifying here.

Same for OnPageHide.

>Index: content/base/src/nsDocument.cpp

>+          link->LinkAdded();

I assume we just need this so we can reuse the event-dispatching code in
nsHTMLLinkElement?  Could we (and do we want to?) get away with just unbinding
and rebinding the content node instead of adding things to the nsILink api that
don't really apply to all links?  If not, I guess this is ok.

>Index: content/html/content/public/nsILink.h

If we do add methods to nsILink, they should probably be NS_IMETHOD; this is a
reasonably "real" XPCOM interface, one that we may even want to move into IDL.

>+nsDocShell::BeginRestore(PRBool aTop)

>+    // Dispatch events for restoring the presentation.  We try to simulate
>+    // the effect of loading the document, so we add the document's channel
>+    // to the loadgroup to initiate stateChange notifications.

Maybe "simulate the progress notifications loading the document would cause"? 
That would make it clearer why we're calling BeginRestore() on the kids, I
think.

>+            child->BeginRestore(PR_FALSE);

If that fails (eg out of memory), shouldn't we bail out?

>+        if (NS_FAILED(rv)) {
>+            PL_DestroyEvent(evt);
>+        }
>+    }
>+    return NS_OK;

And shouldn't failure to post that event be propagated out?

The rest looks good to me.  sr=me with the nits; I'd still like sicking to take
a look at the DOM/content changes, though, since I didn't read the code that
carefully.

Comment 20

[Brian Ryner (not reading)]

(In reply to comment #19)
> I assume we just need this so we can reuse the event-dispatching code in
> nsHTMLLinkElement?  Could we (and do we want to?) get away with just unbinding
> and rebinding the content node instead of adding things to the nsILink api that
> don't really apply to all links?  If not, I guess this is ok.

Hm, that seems kind of scary to me (unbinding and rebinding the node).  It looks
like, for example, doing this for a style <link> would cause the stylesheet to
be unapplied and then reapplied.  I'd rather keep the explicit method for
handling the notification.

Comment 21

[Brian Ryner (not reading)]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

Requesting approval for this change, which should only impact fastback. 
(sr=bzbarsky; I'll be happy to address any comments sicking has in a followup
patch).

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

So far this is all I have, still reviewing though.

PageChangeEvent might not be an ideal name. Sounds more like the DOM is mutated
then that the page is shown or hidden. How about PageShowHideEvent or
PageTransitionEvent?

We talked over irc about making PageHide always fire before unload. Did you
make that change? (the wiki page still show them the other way around).

>Index: content/html/content/public/nsILink.h
>@@ -86,6 +86,20 @@ public:
>     */
>   NS_IMETHOD GetHrefURI(nsIURI** aURI) = 0;
> 
>+  /**
>+   * Dispatch a LinkAdded event to the nsIChromeEventHandler for this document.
>+   * This is used to notify the chrome listeners when restoring a page
>+   * presentation.  Currently, this only applies to HTML <link> elements.
>+   */
>+  virtual void LinkAdded() = 0;
>+
>+  /**
>+   * Dispatch a LinkRemoved event to the nsIChromeEventHandler for this
>+   * document.  This is used to notify the chrome listeners when saving a page
>+   * presentation (since the document is not torn down).  Currently, this only
>+   * applies to HTML <link> elements.
>+   */
>+  virtual void LinkRemoved() = 0;

You could provide a default implementation here to avoid having to add an empty
function in all classes, we do that in nsIContent for a few functions. Up to
you which you like best.

>Index: docshell/base/nsDocShell.cpp
>@@ -4622,7 +4626,7 @@ nsDocShell::EndPageLoad(nsIWebProgress *
>     // This will cause any OnLoad(...) handlers to fire, if it is a HTML
>     // document...
>     //
>-    if (!mEODForCurrentDocument && mContentViewer) {
>+    if ((mIsRestoringDocument || !mEODForCurrentDocument) && mContentViewer) {
>         mIsExecutingOnLoadHandler = PR_TRUE;
>         mContentViewer->LoadComplete(aStatus);
>         mIsExecutingOnLoadHandler = PR_FALSE;

Would it be a good idea to let nsDocumentViewer::LoadComplete take care of
firing the PageShow event too?

Comment 23

[Brian Ryner (not reading)]

(In reply to comment #22)
> (From update of attachment 186170 [details] [diff] [review] [edit])
> So far this is all I have, still reviewing though.
> PageChangeEvent might not be an ideal name. Sounds more like the DOM is 
mutated
> then that the page is shown or hidden. How about PageShowHideEvent or
> PageTransitionEvent?

I'd be agreeable to Transition if Darin/Boris are, but I'd rather not get into 
a prolonged naming discussion.

> We talked over irc about making PageHide always fire before unload. Did you
> make that change? (the wiki page still show them the other way around).

I did make that change, yes.

> You could provide a default implementation here to avoid having to add an 
empty
> function in all classes, we do that in nsIContent for a few functions. Up to
> you which you like best.

I think I'll leave it without, given Boris's interest in perhaps converting 
this interface to IDL.

> >Index: docshell/base/nsDocShell.cpp
> >@@ -4622,7 +4626,7 @@ nsDocShell::EndPageLoad(nsIWebProgress *
> >     // This will cause any OnLoad(...) handlers to fire, if it is a HTML
> >     // document...
> >     //
> >-    if (!mEODForCurrentDocument && mContentViewer) {
> >+    if ((mIsRestoringDocument || !mEODForCurrentDocument) && 
mContentViewer) {
> >         mIsExecutingOnLoadHandler = PR_TRUE;
> >         mContentViewer->LoadComplete(aStatus);
> >         mIsExecutingOnLoadHandler = PR_FALSE;
> Would it be a good idea to let nsDocumentViewer::LoadComplete take care of
> firing the PageShow event too?

It does... (it calls doc->OnPageShow)

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I'd rather keep the explicit method for handling the notification.

Sounds good.  Good catch on the stylesheets issue.

PageTransitionEvent sounds fine to me.

Comment 25

[Brian Ryner (not reading)]

Changed to PageTransition event, also I added a null check for mainEventQueue
since this can apparently be null during shutdown.

Comment 26

[Mike Shaver (:shaver emeritus)]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

a=shaver.

Comment 27

[Brian Ryner (not reading)]

I optimistically checked in the patch with existing reviews and approval. 
Jonas, if you have any additional review comments I can address those after
checkin, but right now I think we should get this in so that we can determine
which other bugs are fixed by it (should be a few).

Leaving the bug _open_ until review is done.

Comment 28

[Darin Fisher]

I also agree with the event name transition ;-)

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The patch for this is causing a massive amount of assertion spew in
nsCharTraits.h:289:

+  if (eventType.LowerCaseEqualsLiteral("PageHide")) {

LowerCaseEqualsLiteral expects the literal to already be lowercase; I think you
want EqualsIgnoreCase (as used for "PageShow").

(I think there's another instance of this problem in the file, just one not hit
nearly as frequently)

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #29)
> +  if (eventType.LowerCaseEqualsLiteral("PageHide")) {
> 
> LowerCaseEqualsLiteral expects the literal to already be lowercase; I think you
> want EqualsIgnoreCase (as used for "PageShow").

erm, why not just use .LowerCaseEqualsLiteral("pagehide"), which isn't in an
MOZ_STRING_WITH_OBSOLETE_API ifdef (as opposed to EqualsIgnoreCase)?

Comment 31

[Brian Ryner (not reading)]

Attachment: fix string comparison

Comment 32

[Brian Ryner (not reading)]

Checked in the last patch.  I'm going to go ahead and mark this FIXED to avoid
confusion on the dependency list, but superreview on attachment 186170 [details] [diff] [review] is still
welcome.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

marking this r=me. I really did review it back then, just seemed to have added
the final mark

Comment 34

[Kai Engert (:KaiE:)]

I'm tempted to reopen this bug, because it caused a major regression (bug 358438 - sorry, security sensitive). See bug 358438 comment 8 and 9.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I'm not sure how reopening this bug would help fix that one...  Generally regressions are fixed in separate bugs that track them.