bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Sort out the progress listener story with fastback/bfcache

RESOLVED FIXED

Status

()

Core
Document Navigation
--
major
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: bz, Assigned: Brian Ryner (not reading))

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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

13 years ago
> 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.
Created attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

jst, what do you think?
Attachment #182749 - Flags: superreview?(jst)
Attachment #182749 - Flags: review?(jst)
Comment on attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

Yeah, fine with me. r+sr=jst
Attachment #182749 - Flags: superreview?(jst)
Attachment #182749 - Flags: superreview+
Attachment #182749 - Flags: review?(jst)
Attachment #182749 - Flags: review+
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.
Attachment #182749 - Flags: approval1.8b3?

Comment 5

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

a=chofmann,  boris, did you mean 1.8b2?
Attachment #182749 - Flags: approval1.8b3?
Attachment #182749 - Flags: approval1.8b3+
Attachment #182749 - Flags: approval1.8b2+
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 on attachment 182749 [details] [diff] [review]
Push the mChannel up to nsDocument, expose on nsIDocument

Checked in this change.
Attachment #182749 - Attachment is obsolete: true

Updated

13 years ago
Blocks: 297155
No longer blocks: 297155
(Assignee)

Comment 8

13 years ago
Created attachment 185995 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Assignee: nobody → bryner
Status: NEW → ASSIGNED
Attachment #185995 - Flags: superreview?(bzbarsky)
Attachment #185995 - Flags: review?(darin)

Comment 9

13 years ago
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.
Attachment #185995 - Flags: review?(darin) → review-
(Assignee)

Comment 10

13 years ago
(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.
(Assignee)

Comment 11

13 years ago
Created attachment 186163 [details] [diff] [review]
patch w/ darin's comments addressed

Addressed darin's comments except as noted above
Attachment #185995 - Attachment is obsolete: true
Attachment #186163 - Flags: superreview?(bzbarsky)
Attachment #186163 - Flags: review?(darin)

Comment 12

13 years ago
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.
Attachment #186163 - Flags: review?(darin) → review-
(Assignee)

Comment 13

13 years ago
Created attachment 186170 [details] [diff] [review]
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)
Attachment #186163 - Attachment is obsolete: true
Attachment #186170 - Flags: superreview?(bugmail)
Attachment #186170 - Flags: review?(darin)

Comment 14

13 years ago
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() ?
Attachment #186170 - Flags: review?(darin) → review+
(Assignee)

Comment 15

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

I made that change locally (revoke events before contentviewer->Stop())
Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

Sorry, I'm not a superreviewer.
Attachment #186170 - Flags: superreview?(bugmail)

Comment 17

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

Jonas: pretend that I gave SR :-)
Attachment #186170 - Flags: superreview?(bugmail)

Updated

13 years ago
Attachment #185995 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 18

13 years ago
Jonas: Yes, I'm more interested in you or Boris looking at the code than I am
the formalities of our review process.
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.
(Assignee)

Comment 20

13 years ago
(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.
(Assignee)

Comment 21

13 years ago
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).
Attachment #186170 - Flags: approval1.8b3?
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?
(Assignee)

Comment 23

13 years ago
(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)
> I'd rather keep the explicit method for handling the notification.

Sounds good.  Good catch on the stylesheets issue.

PageTransitionEvent sounds fine to me.
(Assignee)

Comment 25

13 years ago
Changed to PageTransition event, also I added a null check for mainEventQueue
since this can apparently be null during shutdown.
Comment on attachment 186170 [details] [diff] [review]
revoke pending events on Stop()

a=shaver.
Attachment #186170 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 27

13 years ago
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

13 years ago
I also agree with the event name transition ;-)
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)
(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)?
(Assignee)

Comment 31

13 years ago
Created attachment 186635 [details] [diff] [review]
fix string comparison
Attachment #186635 - Flags: review?(darin)
Attachment #186635 - Flags: approval1.8b3?

Updated

13 years ago
Attachment #186635 - Flags: review?(darin) → review+

Updated

13 years ago
Blocks: 297942
Attachment #186163 - Flags: superreview?(bzbarsky)

Updated

13 years ago
Attachment #186635 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 32

13 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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
Attachment #186170 - Flags: superreview?(bugmail) → superreview+

Comment 34

11 years ago
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.
I'm not sure how reopening this bug would help fix that one...  Generally regressions are fixed in separate bugs that track them.
Depends on: 424646

Updated

10 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell

Updated

8 years ago
Depends on: 564702
You need to log in before you can comment on or make changes to this bug.