From 7797c2369e23337534050efc1113b98f993cfe9a Mon Sep 17 00:00:00 2001 From: poire-z Date: Wed, 3 Jan 2018 09:43:49 +0100 Subject: [PATCH] credocument: deal with loadDocument() failures (#3570) cre:loadDocument() may fail in recognizing the document format, and koreader would previously keep calling other methods on it, which would make crengine segfaults. We now check loadDocument success at the various places it is called, and try to deal the best way we can when it fails. --- base | 2 +- .../apps/filemanager/filemanagerbookinfo.lua | 43 +++--- frontend/apps/reader/readerui.lua | 34 ++++- frontend/document/credocument.lua | 15 ++- .../coverbrowser.koplugin/bookinfomanager.lua | 125 +++++++++--------- 5 files changed, 140 insertions(+), 79 deletions(-) diff --git a/base b/base index b7342e6aa..81e789e72 160000 --- a/base +++ b/base @@ -1 +1 @@ -Subproject commit b7342e6aa475876b5c95440ca24278aacba6eb85 +Subproject commit 81e789e724c7417c1691652532c3ea0a574cba2a diff --git a/frontend/apps/filemanager/filemanagerbookinfo.lua b/frontend/apps/filemanager/filemanagerbookinfo.lua index 4824480a4..2f3c6fd87 100644 --- a/frontend/apps/filemanager/filemanagerbookinfo.lua +++ b/frontend/apps/filemanager/filemanagerbookinfo.lua @@ -98,23 +98,36 @@ function BookInfo:show(file, book_props) -- If still no book_props (book never opened or empty 'stats'), open the -- document to get them if not book_props then - local pages local document = DocumentRegistry:openDocument(file) - if document.loadDocument then -- needed for crengine - document:loadDocument() - -- document:render() - -- It would be needed to get nb of pages, but the nb obtained - -- by simply calling here document:getPageCount() is wrong, - -- often 2 to 3 times the nb of pages we see when opening - -- the document (may be some other cre settings should be applied - -- before calling render() ?) - else - -- for all others than crengine, we seem to get an accurate nb of pages - pages = document:getPageCount() + if document then + local loaded = true + local pages + if document.loadDocument then -- CreDocument + if not document:loadDocument() then + -- failed loading, calling other methods would segfault + loaded = false + end + -- For CreDocument, we would need to call document:render() + -- to get nb of pages, but the nb obtained by simply calling + -- here document:getPageCount() is wrong, often 2 to 3 times + -- the nb of pages we see when opening the document (may be + -- some other cre settings should be applied before calling + -- render() ?) + else + -- for all others than crengine, we seem to get an accurate nb of pages + pages = document:getPageCount() + end + if loaded then + book_props = document:getProps() + book_props.pages = pages + end + DocumentRegistry:closeDocument(file) end - book_props = document:getProps() - book_props.pages = pages - DocumentRegistry:closeDocument(file) + end + + -- If still no book_props, fall back to empty ones + if not book_props then + book_props = {} end local title = book_props.title diff --git a/frontend/apps/reader/readerui.lua b/frontend/apps/reader/readerui.lua index a307039c5..c4f3d4748 100644 --- a/frontend/apps/reader/readerui.lua +++ b/frontend/apps/reader/readerui.lua @@ -263,7 +263,9 @@ function ReaderUI:init() else -- make sure we render document first before calling any callback self:registerPostInitCallback(function() - self.document:loadDocument() + if not self.document:loadDocument() then + self:dealWithLoadDocumentFailure() + end -- used to read additional settings after the document has been -- loaded (but not rendered yet) @@ -561,4 +563,34 @@ function ReaderUI:onClose() end end +function ReaderUI:dealWithLoadDocumentFailure() + -- Sadly, we had to delay loadDocument() to about now, so we only + -- know now this document is not valid or recognized. + -- We can't do much more than crash properly here (still better than + -- going on and segfaulting when calling other methods on unitiliazed + -- _document) + -- We must still remove it from lastfile and history (as it has + -- already been added there) so that koreader don't crash again + -- at next launch... + local readhistory = require("readhistory") + readhistory:removeItemByPath(self.document.file) + if G_reader_settings:readSetting("lastfile") == self.document.file then + G_reader_settings:saveSetting("lastfile", #readhistory.hist > 0 and readhistory.hist[1].file or nil) + end + -- As we are in a coroutine, we can pause and show an InfoMessage before exiting + local _coroutine = coroutine.running() + if coroutine then + logger.warn("crengine failed recognizing or parsing this file: unsupported or invalid document") + UIManager:show(InfoMessage:new{ + text = _("Failed recognizing or parsing this file: unsupported or invalid document.\nKOReader will exit now."), + dismiss_callback = function() + coroutine.resume(_coroutine, false) + end, + }) + coroutine.yield() -- pause till InfoMessage is dismissed + end + -- We have to error and exit the coroutine anyway to avoid any segfault + error("crengine failed recognizing or parsing this file: unsupported or invalid document") +end + return ReaderUI diff --git a/frontend/document/credocument.lua b/frontend/document/credocument.lua index 9f3623b49..984a40689 100644 --- a/frontend/document/credocument.lua +++ b/frontend/document/credocument.lua @@ -109,6 +109,11 @@ function CreDocument:init() -- set fallback font face self._document:setStringProperty("crengine.font.fallback.face", self.fallback_font) + -- We would have liked to call self._document:loadDocument(self.file) + -- here, to detect early if file is a supported document, but we + -- need to delay it till after some crengine settings are set for a + -- consistent behaviour. + self.is_open = true self.info.has_pages = false self:_readMetadata() @@ -117,9 +122,11 @@ end function CreDocument:loadDocument() if not self._loaded then - self._document:loadDocument(self.file) - self._loaded = true + if self._document:loadDocument(self.file) then + self._loaded = true + end end + return self._loaded end function CreDocument:render() @@ -160,7 +167,9 @@ end function CreDocument:getCoverPageImage() -- don't need to render document in order to get cover image - self:loadDocument() + if not self:loadDocument() then + return nil -- not recognized by crengine + end local data, size = self._document:getCoverPageImageData() if data and size then local Mupdf = require("ffi/mupdf") diff --git a/plugins/coverbrowser.koplugin/bookinfomanager.lua b/plugins/coverbrowser.koplugin/bookinfomanager.lua index 11faa81c8..26f2d479f 100644 --- a/plugins/coverbrowser.koplugin/bookinfomanager.lua +++ b/plugins/coverbrowser.koplugin/bookinfomanager.lua @@ -363,78 +363,85 @@ function BookInfoManager:extractBookInfo(filepath, cover_specs) -- Proceed with extracting info local document = DocumentRegistry:openDocument(filepath) + local loaded = true if document then + local pages if document.loadDocument then -- needed for crengine -- Setting a default font before loading document -- actually do prevent some crashes document:setFontFace(document.default_font) - document:loadDocument() - -- Not needed for getting props: - -- document:render() - -- It would be needed to get nb of pages, but the nb obtained - -- by simply calling here document:getPageCount() is wrong, - -- often 2 to 3 times the nb of pages we see when opening - -- the document (may be some other cre settings should be applied - -- before calling render() ?) + if not document:loadDocument() then + -- failed loading, calling other methods would segfault + loaded = false + end + -- For CreDocument, we would need to call document:render() + -- to get nb of pages, but the nb obtained by simply calling + -- here document:getPageCount() is wrong, often 2 to 3 times + -- the nb of pages we see when opening the document (may be + -- some other cre settings should be applied before calling + -- render() ?) else -- for all others than crengine, we seem to get an accurate nb of pages - local pages = document:getPageCount() - dbrow.pages = pages - end - local props = document:getProps() - if next(props) then -- there's at least one item - dbrow.has_meta = 'Y' + pages = document:getPageCount() end - if props.title and props.title ~= "" then dbrow.title = props.title end - if props.authors and props.authors ~= "" then dbrow.authors = props.authors end - if props.series and props.series ~= "" then dbrow.series = props.series end - if props.language and props.language ~= "" then dbrow.language = props.language end - if props.keywords and props.keywords ~= "" then dbrow.keywords = props.keywords end - if props.description and props.description ~= "" then dbrow.description = props.description end - if cover_specs then - local spec_sizetag = cover_specs.sizetag - local spec_max_cover_w = cover_specs.max_cover_w - local spec_max_cover_h = cover_specs.max_cover_h - - dbrow.cover_fetched = 'Y' -- we had a try at getting a cover - -- XXX make picdocument return a blitbuffer of the image - local cover_bb = document:getCoverPageImage() - if cover_bb then - dbrow.has_cover = 'Y' - dbrow.cover_sizetag = spec_sizetag - -- we should scale down the cover to our max size - local cbb_w, cbb_h = cover_bb:getWidth(), cover_bb:getHeight() - local scale_factor = 1 - if cbb_w > spec_max_cover_w or cbb_h > spec_max_cover_h then - -- scale down if bigger than what we will display - scale_factor = math.min(spec_max_cover_w / cbb_w, spec_max_cover_h / cbb_h) - cbb_w = math.min(math.floor(cbb_w * scale_factor)+1, spec_max_cover_w) - cbb_h = math.min(math.floor(cbb_h * scale_factor)+1, spec_max_cover_h) - local new_bb - if self.use_legacy_image_scaling then - new_bb = cover_bb:scale(cbb_w, cbb_h) - else - new_bb = Mupdf.scaleBlitBuffer(cover_bb, cbb_w, cbb_h) + if loaded then + dbrow.pages = pages + local props = document:getProps() + if next(props) then -- there's at least one item + dbrow.has_meta = 'Y' + end + if props.title and props.title ~= "" then dbrow.title = props.title end + if props.authors and props.authors ~= "" then dbrow.authors = props.authors end + if props.series and props.series ~= "" then dbrow.series = props.series end + if props.language and props.language ~= "" then dbrow.language = props.language end + if props.keywords and props.keywords ~= "" then dbrow.keywords = props.keywords end + if props.description and props.description ~= "" then dbrow.description = props.description end + if cover_specs then + local spec_sizetag = cover_specs.sizetag + local spec_max_cover_w = cover_specs.max_cover_w + local spec_max_cover_h = cover_specs.max_cover_h + + dbrow.cover_fetched = 'Y' -- we had a try at getting a cover + -- XXX make picdocument return a blitbuffer of the image + local cover_bb = document:getCoverPageImage() + if cover_bb then + dbrow.has_cover = 'Y' + dbrow.cover_sizetag = spec_sizetag + -- we should scale down the cover to our max size + local cbb_w, cbb_h = cover_bb:getWidth(), cover_bb:getHeight() + local scale_factor = 1 + if cbb_w > spec_max_cover_w or cbb_h > spec_max_cover_h then + -- scale down if bigger than what we will display + scale_factor = math.min(spec_max_cover_w / cbb_w, spec_max_cover_h / cbb_h) + cbb_w = math.min(math.floor(cbb_w * scale_factor)+1, spec_max_cover_w) + cbb_h = math.min(math.floor(cbb_h * scale_factor)+1, spec_max_cover_h) + local new_bb + if self.use_legacy_image_scaling then + new_bb = cover_bb:scale(cbb_w, cbb_h) + else + new_bb = Mupdf.scaleBlitBuffer(cover_bb, cbb_w, cbb_h) + end + cover_bb:free() + cover_bb = new_bb end - cover_bb:free() - cover_bb = new_bb + dbrow.cover_w = cbb_w + dbrow.cover_h = cbb_h + dbrow.cover_btype = cover_bb:getType() + dbrow.cover_bpitch = cover_bb.pitch + local cover_data = Blitbuffer.tostring(cover_bb) + cover_bb:free() -- free bb before compressing to save memory + dbrow.cover_datalen = cover_data:len() + local cover_dataz = xutil.zlib_compress(cover_data) + -- release memory used by uncompressed data: + cover_data = nil -- luacheck: no unused + dbrow.cover_dataz = SQ3.blob(cover_dataz) -- cast to blob for sqlite + logger.dbg("cover for", filename, "scaled by", scale_factor, "=>", cbb_w, "x", cbb_h, "(compressed from ", dbrow.cover_datalen, " to ", cover_dataz:len()) end - dbrow.cover_w = cbb_w - dbrow.cover_h = cbb_h - dbrow.cover_btype = cover_bb:getType() - dbrow.cover_bpitch = cover_bb.pitch - local cover_data = Blitbuffer.tostring(cover_bb) - cover_bb:free() -- free bb before compressing to save memory - dbrow.cover_datalen = cover_data:len() - local cover_dataz = xutil.zlib_compress(cover_data) - -- release memory used by uncompressed data: - cover_data = nil -- luacheck: no unused - dbrow.cover_dataz = SQ3.blob(cover_dataz) -- cast to blob for sqlite - logger.dbg("cover for", filename, "scaled by", scale_factor, "=>", cbb_w, "x", cbb_h, "(compressed from ", dbrow.cover_datalen, " to ", cover_dataz:len()) end end DocumentRegistry:closeDocument(filepath) - else + end + if not loaded then dbrow.unsupported = _("not readable by engine") dbrow.cover_fetched = 'Y' -- so we don't try again if we're called later if cover_specs end