Antergos / web-greeter

A modern, visually appealing greeter for LightDM.
http://antergos.github.io/web-greeter
GNU General Public License v3.0
232 stars 57 forks source link

RFC :: Theme API :: Require Themes To Provide An Init Callback #104

Open lots0logs opened 7 years ago

lots0logs commented 7 years ago

Currently it is very difficult for the greeter to detect and respond to errors that might occur in the themes. I've tried to tackle this problem a few different ways over the last two years with little success. The bottom line is that, currently it's impossible for the greeter to properly detect theme errors 100% of the time. It's a guessing game at best and the guessing degrades the overall user experience.

One possible solution that I've been considering is to require that themes provide an init callback function to the greeter and ensure that nothing in the theme starts until that callback is called. That way, from the greeter's side we can use a try/catch block to properly detect errors that the theme didn't catch/handle. I'd love to hear from theme developers on this. What do you think? Any questions, comments, or concerns?

cc: @FallingSnow @Litarvan @tomasantunes @pedropenna @NoiSek @rufuswilson @legostax @codehearts @shosca @The5heepDev @kasperrt

codehearts commented 7 years ago

How would the greeter handle an error thrown from init? Would it simply be logged, or would the greeter be able to somehow try to recover?

From a theming standpoint, this doesn't sound like a lot of work to implement. I'm just curious how this could detect errors on event listeners and other event-driven code registered in init, since most theme code is run in callbacks.

lots0logs commented 7 years ago

Basically what the greeter will do is alert the user that an error was detected and give them the option to load one of the default themes (thus preventing them from being stuck, unable to log in, due to a theme issue).

tyrumus commented 7 years ago

I would have loved this feature when I was developing my theme a few months ago. Every time my code had an error that rendered the login unusable, I had to switch ttys, change the .conf file to point to a working theme, and reboot. As far as detecting errors though, I made a makeshift devtools <div> and printed my console errors there. It would also be nice to have the greeter log Javascript errors to a file so the user can read them once they're logged in. (forgive me if this is implemented already)

codehearts commented 7 years ago

Sorry if this has already been addressed, but would the greeter be able to handle errors thrown in event listeners? I'm not sure that this solution could catch errors in say, a keypress handler that switches users.

lots0logs commented 7 years ago

@legostax

It would also be nice to have the greeter log Javascript errors to a file so the user can read them once they're logged in.

Indeed, its already implemented. All you have to do is enable debug mode in your greeter config file :wink:

@codehearts Provided that the init hook callback function is the only entry point into the theme's code execution the greeter will be able to catch any errors that are not caught and handled by the theme.

FallingSnow commented 7 years ago

I will argue that the current error detection system has given me numerous false alarms, but I have a few questions.

Also... I really feel like this is a hacky way to fix the issue. I assume you have looked into webkit2gtk's inability to detect errors accurately in depth. Is the fix so hard (or perhaps the reporting, I remember what you said about opening issues with webkitgtk) that we should resort to something like this? I have suspicions that this "hack" will be just as unreliable.... Just food for thought.

lots0logs commented 7 years ago

@FallingSnow

I will argue that the current error detection system has given me numerous false alarms,

Indeed, that's what the first paragraph in my OC is referring to.

Will a try/catch really be able to catch any error thrown in the theme?

Yes. Exceptions bubble up the call stack until they are caught or reach the top. If the theme ensures that its only entry point is a single callback function set on the window object (this will be well documented before its required, btw) then the greeter will be able to wrap the execution of the theme so that any errors that the theme doesn't handle will be caught by the greeter. There would be no guessing involved in this case. In fact no webkitgtk APIs would even be involved. This would all be handled completely in JavaScript using constructs native to the language.

I'm not sure what your jsfiddle is intended to show? Could you explain it?

What if an error occurs in the greeter that breaks the user's ability to login, is that already handled?

That's always a possibility though it has only happened once since I took over the project three years ago (I'm sure you recall the localStorage fiasco :unamused: ) The greeter's codebase is held to pretty high standards (as it should be), but remember that we have no control over 3rd-party themes' code. That is the problem area that I want to improve. I basically want there never to be a case where the user is stuck, unable to log in to their system.

I really feel like this is a hacky way to fix the issue.

I'm a bit confused. This is not a hacky solution. Its actually pretty common practice for any software application that allows itself to be extended, enhanced, or in some way modified by external, 3rd-party code. Surely, you've heard of the concept of hooks/actions/filters? If not, here's some good info (its php but the concept applies in all programming languages) :wink:

FallingSnow commented 7 years ago

I'm not sure what your jsfiddle is intended to show? Could you explain it?

Sorry. I knew I should have added a better example. If any async error happens within the try/catch it will not be caught. If you look through the console you'll see the synchronous error is caught while the asynchronous error is not. I think this is what @codehearts is referring to.

I'm a bit confused. This is not a hacky solution.

By hacky I'm referring to the possible problem with it referenced by the jsfiddle above. Also since I feel the proper solution would be what you tried in your first attempt, the Webkitgtk error API.

lots0logs commented 7 years ago

I see. You're right. Let me give it some more thought...

NoiSek commented 7 years ago

I think this is a very good idea, as someone who is often locked out of their own system while testing during theme development.

It might be more effective to require that themes respond to a 'heartbeat' callback every n milliseconds than to assume 100% integrity upon initial launch, however. Detecting when a theme has stopped responding is probably more reliable than assuming one can catch the error (and also accounts for the edge case where there is no technical error, but the theme is still stuck in a hung state).

Even falling back to a TTY-esque username / password login would be far preferable to the current behavior, so I'm definitely a strong +1 to this proposal.

FallingSnow commented 7 years ago

I believe lots0logs has tried a heartbeat solution before, I don't know how it turned out.

I think the issue is how to actually determine an error has occurred. A heartbeat is good to determine if the theme has crashed entirely, but not if document.createElement('loginBtn') has failed.

lots0logs has the correct approach to watch for all errors across the theme IMO, but how do we go about catching those?

@lots0logs Have you looked into using window.onerror?

NoiSek commented 7 years ago

Presumably the heartbeat would fail to respond in all the same situations that would normally halt program execution anyway, though, so more or less the same general outcome but polled rather than checked once.

In your example, I'm not sure there's any situation where a theme provided heartbeat would fail to report correctly that would also not fail to be caught by a try / catch loop.

That said, no sense in re-inventing the wheel if hooking into window.onerror could potentially solve the problem on its own.

FallingSnow commented 7 years ago

That said, no sense in re-inventing the wheel if hooking into window.onerror could potentially solve the problem on its own.

Agreed.

Presumably the heartbeat would fail to respond in all the same situations that would normally halt program execution anyway, though, so more or less the same general outcome but polled rather than checked once.

Not necessarily. I can come up with a couple reasons as to why I would be opposed to a heartbeat:

Fundamentally, I feel a heartbeat falls victim to the same problem as a try/catch, async errors.

ozwaldorf commented 7 years ago

@lots0logs One thing you could do is have a callback once everything is properly loaded. If the callback isn't triggered, reload the greeter with a fallback theme that works.

codehearts commented 7 years ago

@The5heepDev That won't work in cases where an async function throws an exception. Imagine your code loads just fine and calls that callback function. When the user presses enter to submit their password for authentication, the onSubmit handler throws an exception (maybe the developer misspelled the id of their password field in a getElementById call and are accessing .value on null data). That callback's already been called, so everything's fine as far as the webkit greeter is concerned, and the user is effectively locked out.

FallingSnow commented 7 years ago

Did some testing with window.onerror today. Looks like it might be viable. Didn't generate any false positives with material2.

window.onerror = function(msg, url, lineNumber) {
    console.error('Error thrown:', msg); // Display dialog for fallback theme
};

This method has the benefit/disadvantage of only catching errors after the greeter is initialized, that way we can be sure at least that the error is occurring after the greeter is initialized.

On a side note: I think it would be important to place the popup window in the corner and still allow click through so someone can ignore it.