New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure Landmarks runs correctly when navigating the history #202

Open
matatk opened this Issue Sep 12, 2018 · 1 comment

Comments

1 participant
@matatk
Owner

matatk commented Sep 12, 2018

This seems to work fine on Chrome-like browsers (as others have noted in the bug report linked below). However, on Firefox, sometimes when going back/forward, the content script doesn't bootstrap again. This is because the page was only hidden, not destroyed and re-loaded, so the content script was not re-injected. Therefore the method for detecting that situation, proposed in the bug report, should be adopted.

@matatk matatk added the bug label Sep 12, 2018

@matatk matatk added this to the 2.4.0 milestone Sep 12, 2018

@matatk matatk self-assigned this Sep 12, 2018

matatk added a commit that referenced this issue Sep 14, 2018

Attempt to fix background comms and navigation on Firefox
Note: there are still problems with the workaround for content script
	  non-reinjection on Firefox, so whilst the solution below seems
	  good, it doesn't actually work yet; still getting "WebExtension
	  context not found!" errors:
	  <https://bugzilla.mozilla.org/show_bug.cgi?id=1390715#c6>.

* Change the reconnection logic to use @lydell's method for coping with
  the background script not already being loaded on Firefox -
  https://bugzilla.mozilla.org/show_bug.cgi?id=1474727#c3 - thanks; I
  had been using an approach whereby it would try to reconnect n times
  in quick succession, but timing-based solutions are a bit yucky,
  whereas your method is elegant :-). Addresses #201.
* At least temporarily reverse the changes in 6ea4e56 to make the code
  simpler.
* Use @rpl's method for supporting the content script not being
  re-injected when navigating back to a previous page on Firefox -
  https://bugzilla.mozilla.org/show_bug.cgi?id=1390715#c4 - also thanks;
  I was really stuck with this and would certainly not arrived at the
  same conclusion and clean approach to the workaround :-). Ref #202.
* Reinstate some of the background script logging from b766e49 to help
  with the content script re-injection/context errors ongoing
  investigation.
* Add a FIXME to sort regarding DevTools being persistent across reloads
  on Chrome-like browsers.
@matatk

This comment has been minimized.

Owner

matatk commented Oct 20, 2018

I've tried this with both the in-devleopment 2.4.0 code and the released 2.3.1 code and get the same problem, so the cause is not down to the move from message-passing to port-based communications. It seems to be related to Firefox's "back-forward cache" ("bfcache") and I have added some info to some Bugzilla bugs, but not seen much movement on them, though there were some helpful suggestions from the bugs' reporters.

Ideally I would have the time and expertise to really get to the bottom of it (thanks @graemecoleman for the suggestion of actually modifying ExtensionParent.jsm to add debugging info) and maybe file a new bug specifically for whatever the root problem is. However, at the moment there are other Landmarks features (and other projects) I'd like to work on (and would likely be better at), so will leave it for now.

This only happens when going back in the history a certain number of steps, and can be mitigated by reloading (though Landmarks doesn't tell the user that), so it doesn't seem like a showstopper, though hopefully a solution will come to light soon.

@matatk matatk removed this from the 2.4.0 milestone Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment