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

@@ -1 +1 @@
-Subproject commit b7342e6aa475876b5c95440ca24278aacba6eb85
+Subproject commit 81e789e724c7417c1691652532c3ea0a574cba2a

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

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

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")

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