Skip to content

Fix Menu.append act different from SimpleMenuModel cause crash#7426

Open
waivital wants to merge 2 commits intonwjs:nw46from
waivital:fix-menu-append-behavior
Open

Fix Menu.append act different from SimpleMenuModel cause crash#7426
waivital wants to merge 2 commits intonwjs:nw46from
waivital:fix-menu-append-behavior

Conversation

@waivital
Copy link
Copy Markdown

NWJS Version : nwjs-v0.45.0-beta1-linux-x64.tar.gz
Operating System : Ubuntu 19.10

Expected behavior

Popup menu become empty

Actual behavior

Program crashed

How to reproduce

Use the script below, then right click the body, select Trigger Remove All MenuItem

<script>
var menu = new nw.Menu();

menu.append(new nw.MenuItem({ type: 'separator' }));
menu.append(new nw.MenuItem({
  label: 'Trigger Remove All MenuItem',
  click: function(){
    var i = menu.items.length - 1
    while (i >= 0) {
      menu.removeAt(i)
      i--
    }
  }
}));
menu.append(new nw.MenuItem({ label: 'Item B' }));
menu.append(new nw.MenuItem({ type: 'separator' }));
menu.append(new nw.MenuItem({ type: 'separator' }))
menu.append(new nw.MenuItem({ label: 'Item C' }));

document.body.addEventListener('contextmenu', function(ev) {
  ev.preventDefault();
  menu.popup(ev.x, ev.y);
  return false;
}, false);
</script>

The output

[8784:8784:0327/063840.807725:ERROR:component_loader.cc(169)] Failed to parse extension manifest.
[8784:8784:0327/063840.836020:ERROR:browser_switcher_service.cc(238)] XXX Init()
[8784:8784:0327/063842.557524:FATAL:simple_menu_model.cc(539)] Check failed: static_cast<size_t>(index) < items_.size() (5 vs. 4)
#0 0x7f3ae7c65629 (/nwjs/lib/libnw.so+0x4694628)
Task trace:
#0 0x7f3ae85d07f6 (/nwjs/lib/libnw.so+0x4fff7f5)
#1 0x7f3ae7daf9d6 (/nwjs/lib/libnw.so+0x47de9d5)
IPC message handler context: 0x73E4B09C

[1]    8784 abort (core dumped)  ./nwjs/nw .

@waivital
Copy link
Copy Markdown
Author

During the test, I found a bug will cause unused MenuItem not collected by the gc. I put its fix also in this pr for convenient :D .

Here is the reproducing code

function tryLeak() {
  console.log('Processing...')
  var i = 10000
  while (i > 0) {
    const item = new nw.MenuItem({ label: 'My-b10e17ab-44c9-MenuItem' })
    menu.append(item)
    menu.remove(item)
    i--
  }
  console.log('Done!')
}

The heap snapshot

before run
image

after run
image

@waivital
Copy link
Copy Markdown
Author

waivital commented Jul 2, 2020

@rogerwang can you have a look at the PR? If there's something more required, let me know.

@rogerwang rogerwang changed the base branch from nw45 to nw46 July 3, 2020 12:22
@rogerwang
Copy link
Copy Markdown
Member

Thanks. Will see it soon.

@pragma-git
Copy link
Copy Markdown

I also have this problem at v0.50.1. I have a nagging feeling this is causing some problem on WIndows for my app, whereas MacOS works fine (but Heap MenuItems are not gc:ed here either)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants