bbc / digital-paper-edit-electron

Work in progress - BBC News Labs digital paper edit project - Electron, Cross Platform Desktop app - Mac, Windows, Linux
Other
26 stars 22 forks source link

Transcript editor + Electron crashes the app (white screen) #4

Open pietrop opened 5 years ago

pietrop commented 5 years ago

Describe the bug @bbc/react-transcript-editor + Electron crashes the app (white screen)

This was initially encountered by @greggcorp while correcting text using the app.

To Reproduce Steps to reproduce the behavior:

  1. Go to releases - 1.0.9-alpha.1
  2. Download an install the app.
  3. Get to the transcript view

At this point, is unclear how to reproduce. It's been reported that

Expected behavior Expecting the app not to crash to white screen when correcting text.

Screenshots NA

Additional context I have a hunch that It might be a problem with local storage in electron version of chrome v8 maxing out on space, and not handling that very well.

Transcript editor saves to local storage every time you stop typing. (also see https://github.com/bbc/react-transcript-editor/pull/177#issuecomment-521249604)

To test this hypothesis

Used this snippet from stack overlow Find the maximum length of a single string that can be stored in localStorage and copy and pasted it in the console of electron. with minor modification to remove the dependency from the html element on the page.

+window.result = {}
-window.result = window.result || document.getElementById('result');

for convenience

//Clear localStorage
for (var item in localStorage) delete localStorage[item];

window.result = {}

result.textContent = 'Test running…';

//Start test
//Defer running so DOM can be updated with "test running" message
setTimeout(function () {

    //Variables
    var low = 0,
        high = 2e9,
        half;

    //Two billion may be a little low as a starting point, so increase if necessary
    while (canStore(high)) high *= 2;

    //Keep refining until low and high are equal
    while (low !== high) {
        half = Math.floor((high - low) / 2 + low);

        //Check if we can't scale down any further
        if (low === half || high === half) {
            console.info(low, high, half);
            //Set low to the maximum possible amount that can be stored 
            low = canStore(high) ? high : low;
            high = low;
            break;
        }

        //Check if the maximum storage is no higher than half
        if (storageMaxBetween(low, half)) {
            high = half;
            //The only other possibility is that it's higher than half but not higher than "high"
        } else {
            low = half + 1;
        }

    }

    //Show the result we found!
    result.innerHTML = 'The maximum length of a string that can be stored in localStorage is <strong>' + low + '</strong> characters.';

    //Functions
    function canStore(strLen) {
        try {
            delete localStorage.foo;
            localStorage.foo = Array(strLen + 1).join('A');
            return true;
        } catch (ex) {
            return false;
        }
    }

    function storageMaxBetween(low, high) {
        return canStore(low) && !canStore(high);
    }

}, 0);

Slowly but surely the page went blank, with no error on the console 🤷‍♂

Screenshot 2019-08-30 at 11 10 20

If I do the same thing with the web version of the transcript editor component using the storybook demo there doesn't seem to be the same problem. see console log in screenshot below.

Screenshot 2019-08-30 at 11 17 13

possibly related https://github.com/electron/electron/issues/2357#issuecomment-125813823 from a search for "electron white screen crash"

https://github.com/electron/electron/issues/2357#issuecomment-454553016

emettely commented 5 years ago

Interesting. I wonder if there's some leaky behaviour somewhere that we can look into - i.e. managing state better so not to create too many copies of the transcript... I wonder if we could also increase the storage cap to prevent maxing out

pietrop commented 5 years ago

I think there's already only one copy of the transcript, but yes, re thinking (perhaps even removing) the save to local storage might be a "quick fix". eg there could be a utility to help with saving to local storage, but the logic and responsibility for that could perhaps leave outside of the component? and for this integration with electron could be removed from the client, and just save directly to disk etc..

Overall, the main problem, from what I read, it might be to do with the render process crashing in electron. Which might be to do with local storage maxing out in electron (?) so it might be worth adding this code as explained here https://github.com/electron/electron/issues/2357#issuecomment-454553016

process.on('uncaughtException', (err) => {
        console.log(err);
});

altho not sure, if this is added in the main process, render process or both?

pietrop commented 5 years ago

I tried this

  1. download from autoEdit, releases v1.0.19
  2. open autoEdit
  3. open inspector (shortcut: ⌥ ⌘ i) and click console
  4. paste the code from previous comment https://github.com/bbc/digital-paper-edit-electron/issues/4#issue-487409445
  5. You should see this first 11 and then after a few sec 5242878 5242879 5242878. + The interface is still clickable and usable etc..
Screenshot 2019-09-04 at 11 31 18

So with that info I also tried the following.

  1. cloned this repo
  2. delete node_modules andpackage-lock.json
    rm -rf node_modules
    rm package-lock.json
  3. change package.json version of electron for DPE to be same as autoEdit
...
"devDependencies": {
    "cross-env": "^5.2.0",
-   "electron": "^5.0.6",
+    "electron": "1.6.10",
    "electron-builder": "^20.38.5",
...
  1. npm install
  2. npm start
  3. open console and paste code from previous comment https://github.com/bbc/digital-paper-edit-electron/issues/4#issue-487409445

small caviat, because earlier version of electron does not support ES6 out of the box, the UI of DPE client raises an error around the use of spread operator, but you can open the settings window instead. And try it there. In the mac bar, Speech to Text --> Edit Speech To Text Configuration (or Shotcut⌘ T)

  1. after pasting and running the code, you should find that it runs and doesn't block the UI.

Screenshot 2019-09-04 at 11 39 55

altho after waiting a few seconds, it still only returns 3 and not the memory size as expected 🤷‍♂

pietrop commented 5 years ago

Tried with latest electron, 6.0.7 and get same issue Screenshot 2019-09-04 at 12 06 42

I think it might be good to explore something like this, like adding winston to get a log of what's actually going on before it disconnects

Also tried with electron demo app electron-api-demos and got the same issue. (just to rule out that it's not a DPE specific thing, but that it's an issue with the latest version of electron)

pietrop commented 5 years ago

in electron-main.js you can add

app.on('renderer-process-crashed', function(event) {
  console.log('renderer-process-crashed', event);
});
npm start

paste code from earlier comment https://github.com/bbc/digital-paper-edit-electron/issues/4#issue-487409445 into console

once the dev tools get disconnected in main terminal you get

{
  preventDefault: [Function: preventDefault],
  sender: WebContents {
    webContents: [Circular],
    history: [
      'file:///Users/passarellip/CODE/PERSONAL/DPE/digital-paper-edit-electron/node_modules/@bbc/digital-paper-edit-client/index.html#/'
    ],
    currentIndex: 0,
    pendingIndex: -1,
    inPageIndex: -1,
    _events: [Object: null prototype] {
      'navigation-entry-commited': [Function],
      '-ipc-message': [Function],
      '-ipc-message-sync': [Function],
      'pepper-context-menu': [Function],
      'desktop-capturer-get-sources': [Function],
      'remote-require': [Function],
      'remote-get-global': [Function],
      'remote-get-builtin': [Function],
      'remote-get-current-window': [Function],
      'remote-get-current-web-contents': [Function],
      'remote-get-guest-web-contents': [Function],
      crashed: [Function],
      '-did-get-response-details': [Function],
      '-did-get-redirect-request': [Function],
      'devtools-reload-page': [Function],
      '-new-window': [Function],
      '-add-new-contents': [Function],
      'will-navigate': [Function],
      'did-navigate': [Function],
      destroyed: [Function],
      'devtools-opened': [Function],
      move: [Function],
      activate: [Function],
      'page-title-updated': [Function],
      'render-view-deleted': [Function: listener]
    },
    _eventsCount: 25,
    _maxListeners: 0,
    browserWindowOptions: {
      width: 1000,
      height: 670,
      minWidth: 1000,
      minHeight: 670,
      titleBarStyle: 'show',
      webPreferences: [Object]
    }
  }
}
pietrop commented 5 years ago

Suggested solutions options, make saving to local storage for the @bbc/react-transcript-editor component optional, with default as true to avoid introducing breaking changes, and set it to false in the digital paper edit client.

Possibly add a callback or some kind of trigger to notify on text changes in @bbc/react-transcript-editor, so that logic to auto save can be added from outside of the component. Eg in in DPE client could save directly to server / electron backend.

Relevant issues in transcript editor repo https://github.com/bbc/react-transcript-editor/pull/177#issuecomment-520924060

emettely commented 5 years ago

I would like to understand this a bit more before getting into the solution, so I'm going to install the crash-reporter + a small server to see if I can debug this further.

Setting up breakpad server for Crash Reporter

JFYI the mini-breakpad-server is a bit quirky. When starting the server it'll run on localhost port 1127, but the URL for submitURL is different (see config in crashReporter.start)

  1. git clone mini-breakpad-server
  2. Add symbols downloaded from releases in Electron and place them in pool/symbols directly.
    // running ls in pool/symbols should return
    Electron                       Electron Helper (GPU)          crashpad_handler
    Electron Framework             Electron Helper (Plugin)       libswiftshader_libEGL.dylib
    Electron Helper                Electron Helper (Renderer)     libswiftshader_libGLESv2.dylib
  3. In the main process for electron, add
    
    app.setPath('temp', '/tmp/DPE');

crashReporter.start({ productName: 'DPE_ELECTRON', companyName: 'BBC', submitURL: 'http://127.0.0.1:1127/post', uploadToServer: true });


4. update the `formidable` deps: `"formidable": "^1.2.1",`. (old dependency causes it to fail)
5. Get electron to crash
6. If you open http://127.0.0.1:1127/ in a browser, you should see the crash report
emettely commented 5 years ago

As @pietrop suggested it does look like a localStorage thing. From the crash dump - the last thread 16:

16  Electron Framework!blink::mojom::blink::StorageAreaProxy::Put(WTF::Vector<unsigned char, 0u, WTF::PartitionAllocator> const&, WTF::Vector<unsigned char, 0u, WTF::PartitionAllocator> const&, base::Optional<WTF::Vector<unsigned char, 0u, WTF::PartitionAllocator> > const&, WTF::String const&, base::OnceCallback<void (bool)>) [storage_area.mojom-blink.cc : 1024 + 0xd]`

All the threads in the error stack:

guid: 38aa7ef7-4aaf-4e2e-8a24-b1bfa0aa4cbc
num-experiments: 0

Operating system: Mac OS X
                  10.12.6 16G2128
CPU: amd64
     family 6 model 70 stepping 1
     8 CPUs

Crash reason:  EXC_BREAKPOINT / 0x00000002
Crash address: 0x10ac50417

Thread 0 (crashed)
 0  Electron Framework!mojo::core::NodeChannel::SendChannelMessage(std::__1::unique_ptr<mojo::core::Channel::Message, std::__1::default_delete<mojo::core::Channel::Message> >) [node_channel.cc : 726 + 0x1]
    rbx = 0x00007fa587b3bf50   r12 = 0x00007fa587b1d410
    r13 = 0x00007fa587b380d0   r14 = 0x00007fff57e5b6a0
    r15 = 0x0000000110c87998   rip = 0x000000010ac50417
    rsp = 0x00007fff57e5b5b0   rbp = 0x00007fff57e5b5d0
    Found by: given as instruction pointer in context
 1  Electron Framework!mojo::core::NodeController::SendPeerEvent(mojo::core::ports::NodeName const&, std::__1::unique_ptr<mojo::core::ports::Event, std::__1::default_delete<mojo::core::ports::Event> >) [node_controller.cc : 640 + 0x5]
    rip = 0x000000010ac545d6   rsp = 0x00007fff57e5b5e0
    rbp = 0x00007fff57e5b660
    Found by: stack scanning
 2  Electron Framework!mojo::core::UserMessageImpl::AppendData(unsigned int, unsigned int const*, unsigned int) [user_message_impl.cc : 511 + 0x5]
    rip = 0x000000010ac5d8cf   rsp = 0x00007fff57e5b630
    rbp = 0x00007fff57e5b660
    Found by: stack scanning
 3  Electron Framework!mojo::core::NodeController::ForwardEvent(mojo::core::ports::NodeName const&, std::__1::unique_ptr<mojo::core::ports::Event, std::__1::default_delete<mojo::core::ports::Event> >) [node_controller.cc : 718 + 0x8]
    rip = 0x000000010ac54a87   rsp = 0x00007fff57e5b670
    rbp = 0x00007fff57e5b690
    Found by: stack scanning
 4  Electron Framework!mojo::core::ports::Node::SendUserMessageInternal(mojo::core::ports::PortRef const&, std::__1::unique_ptr<mojo::core::ports::UserMessageEvent, std::__1::default_delete<mojo::core::ports::UserMessageEvent> >*) [node.cc : 862 + 0xe]
    rip = 0x000000010db1b8fb   rsp = 0x00007fff57e5b6a0
    rbp = 0x00007fff57e5b6e0
    Found by: stack scanning
 5  Electron Framework!mojo::core::ports::Node::SendUserMessage(mojo::core::ports::PortRef const&, std::__1::unique_ptr<mojo::core::ports::UserMessageEvent, std::__1::default_delete<mojo::core::ports::UserMessageEvent> >) [node.cc : 367 + 0x5]
    rip = 0x000000010db1b770   rsp = 0x00007fff57e5b6f0
    rbp = 0x00007fff57e5b740
    Found by: stack scanning
 6  Electron Framework!mojo::core::NodeController::SendUserMessage(mojo::core::ports::PortRef const&, std::__1::unique_ptr<mojo::core::ports::UserMessageEvent, std::__1::default_delete<mojo::core::ports::UserMessageEvent> >) [node_controller.cc : 259 + 0x5]
    rip = 0x000000010ac532f1   rsp = 0x00007fff57e5b750
    rbp = 0x00007fff57e5b760
    Found by: stack scanning
 7  Electron Framework!mojo::core::MessagePipeDispatcher::WriteMessage(std::__1::unique_ptr<mojo::core::ports::UserMessageEvent, std::__1::default_delete<mojo::core::ports::UserMessageEvent> >) [message_pipe_dispatcher.cc : 143 + 0x8]
    rip = 0x000000010ac4ea24   rsp = 0x00007fff57e5b770
    rbp = 0x00007fff57e5b780
    Found by: stack scanning
 8  Electron Framework!mojo::core::Core::WriteMessage(unsigned int, unsigned long, MojoWriteMessageOptions const*) [core.cc : 572 + 0xd]
    rip = 0x000000010ac445af   rsp = 0x00007fff57e5b790
    rbp = 0x00007fff57e5b940
    Found by: stack scanning
 9  Electron Framework!void v8::internal::String::WriteToFlat<unsigned char>(v8::internal::String, unsigned char*, int, int) [string.cc : 0 + 0x8]
    rip = 0x000000010b18f41c   rsp = 0x00007fff57e5b7c0
    rbp = 0x00007fff57e5b940
    Found by: stack scanning
10  Electron Framework!base::allocator::MallocZoneFunctionsToReplaceDefault()::$_7::__invoke(_malloc_zone_t*, unsigned long) [allocator_shim.cc : 192 + 0xb]
    rip = 0x000000010c78735d   rsp = 0x00007fff57e5b850
    rbp = 0x00007fff57e5b940
    Found by: stack scanning
11  Electron Framework + 0x2455330
    rip = 0x000000010c787330   rsp = 0x00007fff57e5b858
    rbp = 0x00007fff57e5b940
    Found by: stack scanning
12  Electron Framework!MojoAppendMessageDataImpl [entrypoints.cc : 92 + 0x9]
    rip = 0x000000010ac4b6b5   rsp = 0x00007fff57e5b8d0
    rbp = 0x00007fff57e5b940
    Found by: stack scanning
13  Electron Framework!mojo::Connector::Accept(mojo::Message*) [message_pipe.h : 97 + 0x8]
    rip = 0x000000010ca24883   rsp = 0x00007fff57e5b950
    rbp = 0x00007fff57e5b990
    Found by: stack scanning
14  Electron Framework!mojo::InterfaceEndpointClient::AcceptWithResponder(mojo::Message*, std::__1::unique_ptr<mojo::MessageReceiver, std::__1::default_delete<mojo::MessageReceiver> >) [interface_endpoint_client.cc : 283 + 0x9]
    rip = 0x000000010ca275cb   rsp = 0x00007fff57e5b9a0
    rbp = 0x00007fff57e5ba00
    Found by: stack scanning
15  libsystem_malloc.dylib + 0x1310
    rip = 0x00007fffce47f310   rsp = 0x00007fff57e5b9d0
    rbp = 0x00007fff57e5ba00
    Found by: stack scanning
16  Electron Framework!blink::mojom::blink::StorageAreaProxy::Put(WTF::Vector<unsigned char, 0u, WTF::PartitionAllocator> const&, WTF::Vector<unsigned char, 0u, WTF::PartitionAllocator> const&, base::Optional<WTF::Vector<unsigned char, 0u, WTF::PartitionAllocator> > const&, WTF::String const&, base::OnceCallback<void (bool)>) [storage_area.mojom-blink.cc : 1024 + 0xd]
    rip = 0x000000010ad32e5c   rsp = 0x00007fff57e5ba10
    rbp = 0x00007fff57e5bb90
    Found by: stack scanning

...snip...
emettely commented 5 years ago

https://chromium.googlesource.com/chromium/src/+/master/storage/browser/blob/README.md

This is maybe what it's using under the hood - we can see some caveats there, like "creating the blob too fast"!