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

HttpInspector: new plugin for developpers to inspect KOReader #11457

Merged
merged 11 commits into from Feb 16, 2024

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Feb 8, 2024

Can be used to inspect the state of the objects in a running KOReader.
It can also be used to execute actions (like the ones available to associate to a gesture) with HTTP requests from a remote computer/device/gadget.
The TCP server side is implemented with a new ZeroMQ StreamMessageQueueServer (thanks @bneo99). A small interesting writting about ZeroMQ and doing a small HTTP server that way at http://hintjens.com/blog:42.
Minor UIManager tweak to avoid uneeded inputevent when such a ZeroMQ module is running.

Based on (and supercedes) #10223 by @bneo99 , who did all the groundwork (I just had fun with building simple and pretty HTML and using Lua introspection facilities hacks to present all the objects we developpers are familiar with in a new light :)).
@bneo99: I did some small reworks on the start/stop and menu stuff, please review.

image
Tell me if there are some other "core objects" I didn't think about, stuff not immediately reachable through these 4 ones.

Other but older screenshots available in #10223 (comment) #10223 (comment)

The plugin is enabled by default, but the server is not started by default.
This is to be run only on trusted networks, as it is highly insecure (no httpS, no access restriction, no authentication, all this is out of scope and some additional access checks could be implemented via user patches I think): your wife can see what you read, and you can see what your children read :)

I previously wondered if this kind of plugin (not strictly reading related) should be shipped or better left be a user-plugin, and was ok having it in core if made generic enough (like I think this finally got), and it's probably more interesting for us developpers, and so good to have it handy present by default in our working tree when git clone, without having to download another user-plugin.

Oh, and yes, the HTML content is left untranslatable, it was painful enough :)

-- Nothing below is made available to translators: we output technical details
-- in HTML, for power users and developpers, who should be fine with english.

This change is Reviewable

@hius07
Copy link
Member

hius07 commented Feb 9, 2024

Impressive!
When sending events, sometimes the browser (Windows Chrome) page is not updated, requires the second click on the same item (event is sent, action is done on the device on the first click).
In browseObject() the items are sorted all together via orderedPairs. At first glance, it would be good to see the strings first, sorted separately, and then the tables/functions.
More ideas to follow after further testing, very interesting.

@poire-z
Copy link
Contributor Author

poire-z commented Feb 9, 2024

When sending events, sometimes the browser (Windows Chrome) page is not updated, requires the second click on the same item (event is sent, action is done on the device on the first click).

I also see strange stuff with Microsoft Edge (Chrome based): even just getting on home and click on list of dispatcher event, it just waits, I see a request sent and just waiting for answer in Dev tools, I see in KOReader output that the request went in and the response is printed, so probably sent. If I reclick on that link, the page displays, and there's no 2nd request in the output....
Nothing like this with Firefox. I don't understand what's happening, I don't know if it's a browser bug (or some settings doing more security/malware checks, not familiar with Edge/Chrome), or if it is me missing some HTTP Response headers to make things smoother.
May be some real web developper more familiar with browser pecularities coud do some more serious investigation :/

In browseObject() the items are sorted all together via orderedPairs. At first glance, it would be good to see the strings first, sorted separately, and then the tables/functions.

With "strings first", you mean everything else not a function ? Properties (number or string), tables and object all first but alpha sorted - and then just all the functions alpha-sorted?
Any specific object/URL in mind with lots of the 2 kinds where we could better witness how it looks ?

very interesting.

I thought you'd find that useful.
Still curious, about your dev workflow, if you still only test on your Kindle and don't have any emulator? How do you work and test? Pushing files to your kindle (how?), editing directly on it? USB mounting on & off each time you want to test ? How do you look at the crash.log ? :)

@hius07
Copy link
Member

hius07 commented Feb 9, 2024

With "strings first", you mean everything else not a function ? Properties (number or string), tables and object all first but alpha sorted - and then just all the functions alpha-sorted?
Any specific object/URL in mind with lots of the 2 kinds where we could better witness how it looks ?

Maybe combined by the groups, each group alpha-sorted:

  1. value_type == "string" or value_type == "number" or value_type == "boolean" or value_type == "nil"
  2. value_type == "table"
  3. value_type == "function"
  4. the rest

Example with many items from groups 1 - 3: http://192.168.0.131:8080/koreader/ui/file_chooser/

@hius07
Copy link
Member

hius07 commented Feb 9, 2024

No usb mount. Wifi + SSH server on Kindle give full access to its file system from my PC.
Editing modules on PC, then uploading them to the Kindle.

return {
name = "httpinspector",
fullname = _("HTTP KOReader Inspector"),
description = _([[Allow browsing KOReader internal objects over HTTP. This is aimed at developpers, and may pose some security risks. Only enable this on networks you can trust.]]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description = _([[Allow browsing KOReader internal objects over HTTP. This is aimed at developpers, and may pose some security risks. Only enable this on networks you can trust.]]),
description = _([[Allow browsing KOReader internal objects over HTTP. This is aimed at developers, and may pose some security risks. Only enable this on networks you can trust.]]),

<pre wrap>
<big>Welcome to KOReader inspector HTTP server!</big>

This service is aimed at developpers, <mark>use at your own risk</mark>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This service is aimed at developpers, <mark>use at your own risk</mark>.
This service is aimed at developers, <mark>use at your own risk</mark>.

@Frenzie Frenzie added this to the 2024.02 milestone Feb 9, 2024
@Frenzie Frenzie added the Plugin label Feb 9, 2024
@poire-z
Copy link
Contributor Author

poire-z commented Feb 9, 2024

Maybe combined by the groups, each group alpha-sorted:
value_type == "string" or value_type == "number" or value_type == "boolean" or value_type == "nil"
value_type == "table"
value_type == "function"
the rest

Went with putting "the rest" (the rare cdata & userdata) with the first kind, which would give:
image

I'm fine with that.
I don't think adding an empty line in between kind is needed (the sequence of J on the left for tables feels like enough for visual separation):
image

@bneo99
Copy link

bneo99 commented Feb 9, 2024

@bneo99: I did some small reworks on the start/stop and menu stuff, please review.

Looks good to me!

I tried it on the Kindle and my remote thingy, no issues other than the lack of wakelock when using it for reading, so my Kindle will go to sleep after a while, even though I am still reading. Should we add some sort of wakelock, or handle it by device?

This was what I used to keep the Kindle awake while I am reading (from #10223)

if Device:isKindle() then
    PowerD:resetT1Timeout()
end

@poire-z
Copy link
Contributor Author

poire-z commented Feb 9, 2024

PowerD:resetT1Timeout()

I have no idea what this resetT1Timeout does. You mean you need it after dispatching the event ? Don't you need any kind of stuff like that before, so http requests are accepted and processed ?
I see resetT1Timeout is only used by plugins/autosuspend.koplugin/main.lua.

Should we add some sort of wakelock, or handle it by device?

I was assuming that devices never go into standby, except when explicitely managed by our AutoSuspend (which does autostandby too) plugin, and that other code that want to prevent standby in some section wrap that section or period with UIManager:preventStandby() / UIManager:allowStandby().
I would also think that when wifi is enabled, we may already prevent standby (maybe I'm wrong, and biased because on my Kobo, going into standby while WiFi is enabled causes a crash :)).
So, dunno if we already prevent standby when WiFi is enabled (which is obviously required by your and this plugin :))
But if that's not required for WiFi, I guess it is required by this plugin/server if it wants to be awake when a request comes in, so it's our start()/stop() in here that should call UIManager:preventStandby() / UIManager:allowStandby() - and may be this will snowball into the autosuspend.koplugin, which will then ensure calling resetT1Timeout ?

I'm not really familiar with all that (and don't want to be :)), so pinging @NiLuJe .

@Frenzie
Copy link
Member

Frenzie commented Feb 9, 2024

Wifi in and of itself doesn't prevent standby, but otoh the minimal traffic of an open SSH connection should.

@hius07
Copy link
Member

hius07 commented Feb 10, 2024

The second click is sometimes required not only on sending an event, but on viewing an object too.

Besides that, all features work as expected, encountered no issues.

To show notifications on the reader when sending an event, notifications "From all other sources" must be enabled.

"beforeSuspend",
"initNetworkManager",
"free",
"clear",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding "onResume" allows detecting DeviceListener and NetworkListener.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But gives garbage in BackgroundRunner entry in FileManager ui.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get such garbage elsewhere for all plugins onXYZ methods, as they are wrapped by pluginloader :/

local function sandboxPluginEventHandlers(plugin)
for key, value in pairs(plugin) do
if key:sub(1, 2) == "on" and type(value) == "function" then
plugin[key] = function(self, ...)
local ok, re = pcall(value, self, ...)

(I tried to keep the original method with another name, but it got messy with multiple plugin instantiations :) anyway, I ended up thinking this plugin should may be not do to many adjustments, and just keep telling the truth - even if the truth is ugly sometimes :))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also other modules that have only unique methods, not shared with anything else, ie. ReaderUserHyph.
If we're annoyed with not seeing their name, we could just plug some dummy methods in the classes that need it, ie function ReaderUserHyph:_forClassIdentification() return

@bneo99
Copy link

bneo99 commented Feb 11, 2024

I have no idea what this resetT1Timeout does.

Not exactly sure what it does, but I believe its used to reset the Kindle's own sleep timer. I suspect without this KOReader actually still runs, but as there is no touch input when HTTP requests are sent after ~10 mins the timer will trigger the Kindle to go to sleep.

Can be used to inspect the state of the objects in
a running KOReader.
It can also be used to execute actions (like the ones
available to associate to a gesture) with HTTP requests
from a remote computer/devices/gadgets.
The TCP server side is implemented with a new ZeroMQ
StreamMessageQueueServer (thanks bneo99).
Minor UIManager tweak to avoid uneeded inputevent
when such a ZeroMQ module is running.
@poire-z
Copy link
Contributor Author

poire-z commented Feb 14, 2024

I also see strange stuff with Microsoft Edge (Chrome based): even just getting on home and click on list of dispatcher event, it just waits, I see a request sent and just waiting for answer in Dev tools, I see in KOReader output that the request went in and the response is printed, so probably sent. If I reclick on that link, the page displays, and there's no 2nd request in the output....
Nothing like this with Firefox. I don't understand what's happening, I don't know if it's a browser bug (or some settings doing more security/malware checks, not familiar with Edge/Chrome), or if it is me missing some HTTP Response headers to make things smoother.
May be some real web developper more familiar with browser pecularities coud do some more serious investigation :/

I still can't figure what's happeninng.
tcpdump a request with Edge, I see 2 TCP connections, the request made on the first, then a second TCP connection established, and that 2nd connection getting (by KOReader/ZeroMQ) the response to the first request !

(56807 the local port of the first connection 56808 the local port of the 2nd connection)

09:35:01.587610 IP client.56807 > server.8080 : Flags [S], seq 401295662, win 64240, options [mss 1460,nop,wscale 8,nop,nop,sackOK], length 0
09:35:01.615202 IP client.56807 > server.8080 : Flags [P.], seq 1:493, ack 1, win 1026, length 492: HTTP: GET /koreader/ HTTP/1.1
        0x0000:  4500 0214 a431 4000 8006 40b0 0a00 0002  E....1@...@.....
        0x0010:  0a00 0001 dde7 1f90 17eb 492f 7d54 a64f  ..........I/}T.O
        0x0020:  5018 0402 0f07 0000 4745 5420 2f6b 6f72  P.......GET./kor
        0x0030:  6561 6465 722f 2048 5454 502f 312e 310d  eader/.HTTP/1.1.
        0x0040:  0a48 6f73 743a 2062 7562 6f6e 6963 3a38  .Host:.bubonic:8
        0x0050:  3038 300d 0a43 6f6e 6e65 6374 696f 6e3a  080..Connection:
        0x0060:  206b 6565 702d 616c 6976 650d 0a55 7067  .keep-alive..Upg
        0x0070:  7261 6465 2d49 6e73 6563 7572 652d 5265  rade-Insecure-Re
        0x0080:  7175 6573 7473 3a20 310d 0a55 7365 722d  quests:.1..User-
        0x0090:  4167 656e 743a 204d 6f7a 696c 6c61 2f35  Agent:.Mozilla/5
        0x00a0:  2e30 2028 5769 6e64 6f77 7320 4e54 2031  .0.(Windows.NT.1

09:35:01.628215 IP client.56808 > server.8080 : Flags [S], seq 2780296144, win 64240, options [mss 1460,nop,wscale 8,nop,nop,sackOK], length 0
09:35:01.628361 IP server.8080  > client.56808: Flags [S.], seq 307152198, ack 2780296145, win 64240, options [mss 1460,nop,nop,sackOK,nop,wscale 7], length 0
09:35:01.628672 IP client.56808 > server.8080 : Flags [.], ack 1, win 8212, length 0
09:35:01.645785 IP server.8080  > client.56808: Flags [P.], seq 1:938, ack 1, win 502, length 937: HTTP: HTTP/1.0 200 OK
        0x0000:  4500 03d1 6289 4000 4006 c09b 0a00 0001  E...b.@.@.......
        0x0010:  0a00 0002 1f90 dde8 124e c547 a5b7 f3d1  .........N.G....
        0x0020:  5018 01f6 17c6 0000 4854 5450 2f31 2e30  P.......HTTP/1.0
        0x0030:  2032 3030 204f 4b0d 0a43 6f6e 7465 6e74  .200.OK..Content
        0x0040:  2d54 7970 653a 2074 6578 742f 6874 6d6c  -Type:.text/html
        0x0050:  3b20 6368 6172 7365 743d 7574 662d 380d  ;.charset=utf-8.
        0x0060:  0a43 6f6e 7465 6e74 2d4c 656e 6774 683a  .Content-Length:
        0x0070:  2038 3338 0d0a 436f 6e6e 6563 7469 6f6e  .838..Connection
        0x0080:  3a20 636c 6f73 650d 0a0d 0a3c 6874 6d6c  :.close....<html
        0x0090:  3e0a 3c68 6561 643e 3c74 6974 6c65 3e4b  >.<head><title>K
        0x00a0:  4f52 6561 6465 7220 696e 7370 6563 746f  OReader.inspecto
        0x00b0:  723c 2f74 6974 6c65 3e3c 2f68 6561 643e  r</title></head>
        0x00c0:  0a3c 626f 6479 3e0a 3c70 7265 2077 7261  .<body>.<pre.wra
        0x00d0:  703e 0a3c 6269 673e 5765 6c63 6f6d 6520  p>.<big>Welcome.
09:35:01.696251 IP server.8080  > client.56808: Flags [F.], seq 938, ack 1, win 502, length 0
09:35:01.696399 IP client.56808 > server.8080 : Flags [.], ack 939, win 8208, length 0
09:36:04.443500 IP client.56807 > server.8080 : Flags [F.], seq 493, ack 1, win 1026, length 0
09:36:04.443803 IP server.8080  > client.56807: Flags [F.], seq 1, ack 494, win 501, length 0
09:36:04.444099 IP client.56807 > server.8080 : Flags [.], ack 2, win 1026, length 0

When I hit again the link in Edge, there is no other request/connection, and that content received on the 2nd connection is displayed ... :/
The "id_frame" in StreamMessageQueueServer used to identifiy connection, when logged, is the same (it's not like I got 2 id_frames that I messed up - or ZeroMQ is itself messed un internally...).

@poire-z
Copy link
Contributor Author

poire-z commented Feb 14, 2024

I tried rewriting the tcpserver part without ZMQ, but keeping the same API (so, it can be a drop-in replacement).
I still get these useless multiple connexions from MS Edge (that I can't explain), but at least, KOReader now writes responses on the right one, and I don't get any "hung" request anymore.

@hius07 : can you try with your Chrome how it behaves, if you replace frontend/ui/message/streammessagequeueserver.lua with this file simpletcpserver.lua.txt (rename it to streammessagequeueserver.lua, to not have to update the plugin main.lua). Also please check that KOReader is working all along (I got times when ti must have been stuck in the loop reading from the socket, and the UI was unresponsive, that I hopefully fixed). Thanks.

@poire-z
Copy link
Contributor Author

poire-z commented Feb 14, 2024

This was what I used to keep the Kindle awake while I am reading (from #10223)

if Device:isKindle() then
    PowerD:resetT1Timeout()
end

@bneo99 : where would you insert that in the code of this PR?
I'll probably soon merge this - it should probably later be tweaked for more proper UIManager & autostandby interactions.

@bneo99
Copy link

bneo99 commented Feb 15, 2024

This was what I used to keep the Kindle awake while I am reading (from #10223)

if Device:isKindle() then
    PowerD:resetT1Timeout()
end

@bneo99 : where would you insert that in the code of this PR? I'll probably soon merge this - it should probably later be tweaked for more proper UIManager & autostandby interactions.

Please disregard this code. I tested this part and found that it might be the cause of the random crashes I encountered previously. I will look for another way to keep the Kindle awake, you can proceed to merge this PR without this part.

@hius07
Copy link
Member

hius07 commented Feb 15, 2024

Yeah, this server is good.
I set client:settimeout(0.02, "t") because g_reader_settings/data didn't give the full list.

@poire-z
Copy link
Contributor Author

poire-z commented Feb 15, 2024

Oh, right, our devices over wifi are probably slower :)
Tweaked the timeouts a bit, only the bind/accept() one needs to be small - once something connects, better increase them so we don't abort doing what we're asked for.

@poire-z poire-z merged commit 0506ffe into koreader:master Feb 16, 2024
3 checks passed
@poire-z poire-z deleted the httpinspector branch February 16, 2024 11:24
@poire-z
Copy link
Contributor Author

poire-z commented Feb 16, 2024

The CI "docs" stuff is failing:

[...]
/home/ko/project/plugins/gestures.koplugin/migration.lua:203: no module() call found; no initial doc comment
/home/ko/project/plugins/hello.koplugin/_meta.lua:7: no module() call found; no initial doc comment
/home/ko/project/plugins/httpinspector.koplugin/_meta.lua:7: no module() call found; no initial doc comment
module(...) name deduction failed: base /home/ko/project/frontend/ /home/ko/project/plugins/httpinspector.koplugin/main.lua
Failed to generate documents...

Exited with code exit status 1

It does not mention any of this PR files at the start, but only at the end :) so probably cause by this PR...
(A bit lazy, so asking:) What's the usual problem and the fix ?

@Frenzie
Copy link
Member

Frenzie commented Feb 16, 2024

I don't know if there is a "usual" problem. ;-) But note that all plugins explicitly define a name

--[[--
This is a debug plugin to test Plugin functionality.

@module koplugin.HelloWorld
--]]--

Though in this case it might make more sense to just not make it a docstring? There isn't anything else in that file anyway.

@NiLuJe
Copy link
Member

NiLuJe commented Feb 26, 2024

I have no idea what this resetT1Timeout does.

I see this was resolved later on, but, for completeness' sake: this simply "pings" the native system so as not to let it enter standby on its own, in order for everything to behave as on other platforms (i.e., AutoSuspend decides when to standby).

Nobody should ever be calling this, AutoSuspend already does it as-needed ;).

Comment on lines -1442 to +1448
if self._zeromqs[1] then
self.event_hook:execute("InputEvent")
end
local sent_InputEvent = false
for _, zeromq in ipairs(self._zeromqs) do
for input_event in zeromq.waitEvent, zeromq do
if not sent_InputEvent then
self.event_hook:execute("InputEvent")
sent_InputEvent = true
end
Copy link
Member

@NiLuJe NiLuJe Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, this chunk doesn't make sense to me?

AFAICT, it does the exact same thing as before, but.... uglier? ^^.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent to only send an InputEvent in case waitEvent actually returns something?

Because that raises an interesting conundrum: AFAICT, none of our (ZMQ) waitEvent implementations actually ever return anything... ^^.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may happen when the cat let the mice dance :)
Haven't reread what I wrote to explain the issue and this solution, but will if needed :) It's the 2nd bullet point in the first link, solution detailed in the 2nd link.
#10223 (comment)
#10223 (comment)

As you're back, there's still the Ctrl-C and the "not painting 1 covered widget" that you may have some thoughts about.

Copy link
Member

@NiLuJe NiLuJe Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In no particular order:

  • I can kill stuff just fine via SIGTERM (before this PR, at least ;p) on Kobo, so not quite sure what this was about? (But signals are hell, so I'd rather not have to deal with signals ourselves).
  • The double ^C is the expected behavior for SIGQUIT.
  • Re: CRe partial renderings and this chunk: I guess it was spamming InputEvent because of the active ZMQ, which means it never went "idle" ;).
    The issue, right now, is that since no waitEvent implementations ever return anything, is that: first, this becomes dead code; but more importantly, we no longer send an InputEvent when a ZMQ is active. That's potentially a problem for Calibre Wireless. The main use-case for these is for AutoSuspend's sake, to make it aware that we're actually active and do not want to enter suspend/standby.
    So, while the spam every 50ms is a bit overkill, we do need a way to periodically send those when a ZMQ is in play...
    (And, ideally, at a delay higher than the CRe "idle" threshold, but lower than the lowest AutoSuspend timer... And that gets tricky, because standby can go as low as 2s ^^).
  • Re: the _repaint stuff. This is sort of an artifact of the ZMQ being integrated inside the main/render loop, I don't really have a better way to deal with this.

Copy link
Member

@NiLuJe NiLuJe Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The main use-case for these is for AutoSuspend's sake, to make it aware that we're actually active and do not want to enter suspend/standby.

We might instead want to tackle this the other way around:

  • Extend AutoSuspend to provide a way to stop/resume its timers (a pair of events?), and simply call that whenever it makes sense?

(The framework is already in place because we already do something similar on suspend/resume anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have UIManager:preventStandby() / UIManager:allowStandby() - isn't that what you wish for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's for standby only, not suspend ;).

And, now that I think about it, standby is already skipped if wifi is enabled, which is almost guaranteed for these use-cases ;).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm? A server can happily run locally on the device without any wifi.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hence my "almost guaranteed" comment ^^

@hius07
Copy link
Member

hius07 commented Mar 21, 2024

Use case: https://www.mobileread.com/forums/showthread.php?t=359837.
Dunno is it worth to be mentioned in the User guide @offset-torque

@offset-torque
Copy link

I noted this plugin for adding in the upcoming guide update. I was going to ask for an example what it can be used for. You read my mind hius07 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants