Skip to content
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

Reading a property of null in some cases in SW mode #520

Closed
mossroy opened this issue Jun 4, 2019 · 6 comments
Closed

Reading a property of null in some cases in SW mode #520

mossroy opened this issue Jun 4, 2019 · 6 comments
Assignees
Milestone

Comments

@mossroy
Copy link
Contributor

mossroy commented Jun 4, 2019

When doing some tests in SW mode on Chromium, I had the following error twice :
Cannot read property 'documentElement' of null
which happened on line 909 of app.js

It's a line we recently added. We also added a few null tests, but it looks like we need to add another one

Capture du 2019-06-04 20-30-16

@mossroy mossroy added this to the v2.6 milestone Jun 4, 2019
@mossroy
Copy link
Contributor Author

mossroy commented Jun 4, 2019

I don't remember the exact scenario, but I was switching from a git branch to another branch.
I suspect it might happen when the SW has been modified, and needs to be reloaded by the browser

@Jaifroid
Copy link
Member

Jaifroid commented Sep 5, 2019

Unfortunately I don't think this issue is fixed. In trying to deduplicate code between app.js and service-worker.js in #556, I came across this error, and it seems reproducible on master in Chromium. I cannot reproduce the error on Edge classic or on Firefox Quantum. Therefore I believe it's a timing issue / race condition due to the way Chromium loads the Service Worker. Steps to reproduce on Chromium (running master from localhost):

Simple version

  • Switch to Service Worker mode
  • Open DevTools, load app.js if necessary
  • Completely reload browser by pressing Ctrl-Shift-R inside DevTools, with DevTools open
  • You should see error in screenshot below

Steps to reproduce while completely clearing and reloading Service Worker (to be ultra-sure)

  • Switch to jQuery with DevTools shut
  • Exit browser
  • Open browser and go back to localhost/.../kwix-js/...
  • Ensure you really are in jQuery mode, if not, repeat above, until browser opens from cold start in jQuery mode
  • Open DevTools and unregister any Service Worker for kiwix-js on localhost
  • Completely exit browser, re-open, open DevTools and be sure there is no SW registered
  • Load app.js in DevTools if necessary, and leave it open
  • Switch to Service Worker mode
  • Reload browser completely with Ctrl-Shift-R while DevTools has the focus
  • Error should be reproduced

image

@Jaifroid Jaifroid reopened this Sep 5, 2019
@Jaifroid
Copy link
Member

Jaifroid commented Sep 5, 2019

@mossroy Maybe this error could be solved with code from https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer/ready , e.g.

navigator.serviceWorker.ready.then(function(serviceWorkerRegistration) { ... });

@Jaifroid
Copy link
Member

Jaifroid commented Sep 5, 2019

OK, on https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer/controller I find this:

The controller read-only property of the ServiceWorkerContainer interface returns a ServiceWorker object if its state is activated (the same object returned by ServiceWorkerRegistration.active). This property returns null if the request is a force refresh (Shift + refresh) or if there is no active worker.

Doh! I guess this is to force reload a Service Worker (?) when Devs are testing. Can we therefore ignore? It seems Chromium is the only browser written to spec here, or else I don't know how to issue a force refresh in the other browsers. I'll test a bit more to see if it's only on force refresh that the error occurs.

@mossroy
Copy link
Contributor Author

mossroy commented Sep 6, 2019

I think this is a different problem, as it's not the same variable that is null. You should open another github issue.

@Jaifroid
Copy link
Member

Jaifroid commented Sep 7, 2019

OK, but I think I found the cause of this error (as stated above) since it only occurs on force refresh and only in Chromium. Therefore I think it's an artefact, not a real error (or rather, it's "by design" in Chromium). If it appears in some meaningful way, I'll open an issue. For now, I'll just close this.

@Jaifroid Jaifroid closed this as completed Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants