Running with Fission, and somehow one of my bugzilla tabs has resulted in a bad state where the reload button is not doing anything at all. Other bugzilla tabs are able to reload fine.

Browser console shows:
TypeError: can't access property "requestSHistoryUpdate", this.browser.frameLoader is null
SessionStore.jsm:1019:11
notifySHistoryChanges resource:///modules/sessionstore/SessionStore.jsm:1019
OnHistoryReload resource:///modules/sessionstore/SessionStore.jsm:1057

Nika figured out the STR for this:
With Fission enabled, drag out a working tab out to a new window. The reload on this page now stops working with the error reported in the bug description.

Peter and Nika can reproduce this error.

1. NOTIFY_LISTENERS_CANCELABLE should check an nsresult and not cancel reload if reload listener fails: https://searchfox.org/mozilla-central/rev/c8ce16e4299a3afd560320d8d094556f2b5504cd/docshell/shistory/nsSHistory.cpp#135-136
2. Store browserId instead of browser reference on history listener? Expose GetCurrentTopByBrowserId() to JS through IDL.

Hi Nika or Peter.

I have written a test for this but it only works (sucesfully reproduces the bug) if it sleeps after moving the tab but before clicking reload. There's something that happens in between that causes this bug to happen, and that my test should wait on instead. Do you have some ideas of what it could be?

Thanks.

I'm not 100% sure what's happening here, but perhaps you could do something like:

// After adding tab1, and before swapping...
let prevBrowser = tab1.linkedBrowser;

// ... later, when you're needing to add the sleep:
ok(!prevBrowser.frameLoader, "the swapped-from browser's frameloader has been destroyed");

If that check is failing, it means you're probably not waiting long enough for the browser in the swapped-from tab to become fully destroyed.

(In reply to Nika Layzell [:nika] (ni? for response) from comment #5)

// ... later, when you're needing to add the sleep:
ok(!prevBrowser.frameLoader, "the swapped-from browser's frameloader has been destroyed");

If that check is failing, it means you're probably not waiting long enough for the browser in the swapped-from tab to become fully destroyed.

The check passes, so that's not it.

I'd like to check some things.

(In reply to Chris Peterson [:cpeterson] from comment #2)

Peter and Nika can reproduce this error.

1. NOTIFY_LISTENERS_CANCELABLE should check an nsresult and not cancel reload if reload listener fails: https://searchfox.org/mozilla-central/rev/c8ce16e4299a3afd560320d8d094556f2b5504cd/docshell/shistory/nsSHistory.cpp#135-136

This code should use the nsresult to determine if the JavaScript in the listener threw an exception or exited normally. It should only cancel the reload if it exited normally returning false.

2. Store browserId instead of browser reference on history listener? Expose GetCurrentTopByBrowserId() to JS through IDL.

Is the problem that the browser is replaced when the tab is dragged to a new window? And the browser ID remains the same?

The browser's frameLoader property is null. Is the browser dying? why is its listener still alive?

It looks like I can find the browser id from the browser's browsing context when the listener is created. To turn that into a new browsing context (which I guess is suitable). Nika/Peter suggested using GetCurrentTopByBrowserId() That's a static method on BrowsingContext. How do you export a static method to JS through IDL? I see only non-static methods exported in this way, do I need to make a Service?

(In reply to Paul Bone [:pbone] from comment #7)

I'd like to check some things.

Nika replied via Matrix and I have answers to all of this now.

Depends on D106841

This macro was used in two places to do the same thing. We can remove this
over-generalisation and have clearer code.

Depends on D107803

We can replace this macro with a static function to make the code here
easier to read.

Depends on D107804

Comment on attachment 9208047 [details]
Bug 1691135 - Remove NOTIFY_LISTENERS_CANCELABLE r=mccr8

Revision D107804 was moved to bug 1697737. Setting attachment 9208047 [details] to obsolete.

Comment on attachment 9208048 [details]
Bug 1691135 - Remove the NOTIFY_LISTENERS macro

Revision D107805 was moved to bug 1697737. Setting attachment 9208048 [details] to obsolete.

https://hg.mozilla.org/mozilla-central/rev/2eb1179e274f
https://hg.mozilla.org/mozilla-central/rev/c11964b044fb