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

Jump to bottom

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

Open
wants to merge 2 commits into
base: nw46
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

waivital
Copy link

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
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
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
Member

Thanks. Will see it soon.

@pragma-git
Copy link

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@waivital @rogerwang @pragma-git