MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.03k stars 4.91k forks source link

possible EventEmitter memory leak warning #2122

Closed vincentchu closed 6 years ago

vincentchu commented 7 years ago

Thanks ...

2-am-zzz commented 7 years ago

We've had this warning appear for awhile, so it's nothing new. @kumavis was any of your filter work related to listeners?

danfinlay commented 7 years ago

Probably worth asking: what console are you seeing this warning in? If it's the web page console, that might actually be new.

adklempner commented 7 years ago

I am also getting this error through Google Chrome console. It's only occurred after updating to 3.10.1

It occurs on every page, before MetaMask is injected it seems.

screen shot 2017-09-18 at 4 05 42 pm
danfinlay commented 7 years ago

We are managing event subscriptions differently now, so it's very possible these new warnings are not actual problems, but we'll revisit them, because we should stay quiet if nothing is wrong, especially in the Ðapp's console.

adklempner commented 7 years ago

web3.eth.accounts[0] stops updating after the error occurs. So if MetaMask is locked when the page loads, and is then unlocked, web3.eth.accounts[0] still returns undefined

danfinlay commented 7 years ago

Well that makes it sound much more serious. I just left my computer for a few hours, but we'll be looking at this asap.

vincentchu commented 7 years ago

@flyswatter Thanks for the quick response--- very appreciated. I'm seeing this in the chrome console. FWIW, after this change, I'm no longer able to read events off of my locally deployed smart contracts.

VogtAI commented 7 years ago

For me it's a breaking change. My DApp no longer handles event logs properly without any changes to my code.

vincentchu commented 7 years ago

@VogtAI What were the changes you had to make?

VogtAI commented 7 years ago

@vincentchu I phrased it poorly. I meant to say that I didn't change anything in my code, so I'm sure the MetaMask update broke my DApp.

coopermaruyama commented 7 years ago

Might be related - My MetaMask stopped working all of the sudden today. It crashes right away when I click on the icon. I can confirm that after downgrading to 3.10.0, MetaMask works again. This happens for me only on google chrome canary. works fine in regular chrome browser. Looking at the error logs I see a bunch of the EventEmitter warnings you described.

vsergeev commented 7 years ago

@vincentchu, @VogtAI, :exclamation: same thing happened to me. Event fetching is now broken in my DApp.

Edit: my setup is running with testrpc on http://localhost:8545, event fetch is of the form contractInstance.myEvent(null, {fromBlock: 0, toBlock: 'latest'}, callback). MetaMask version 3.10.0 works fine.

danfinlay commented 7 years ago

We've re-published 3.10.0 as 3.10.2, so your users should have the hot fix soon.

danfinlay commented 7 years ago

We'll be looking closely at the changes and fixing the issue before republishing.

danfinlay commented 7 years ago

We believe we have fixed this issue, could affected developers test this custom build against your dapps, and confirm this fixes this problem?

metamask-chrome-3.10.3-pre.zip

adklempner commented 7 years ago

3.10.3-pre gives me the same issue

BTW, had to remove -pre from the version in manifest.json to manually install it in Chrome

danfinlay commented 7 years ago

@adklempner Could you provide us with reproduction steps? We fixed the issue we were able to reproduce, now we're not so sure.

danfinlay commented 7 years ago

Updated version that fixes the problem @adklempner noted:

metamask-chrome-3.10.3-pre2.zip

vsergeev commented 7 years ago

Thanks for the quick turn around. Unfortunately, no luck with 3.10.3-pre2 for me.

Test.sol

pragma solidity ^0.4.4;

contract Test {
    event Foobar(uint256 x);

    function Test() {
        Foobar(1);
        Foobar(2);
        Foobar(3);
    }
}

app.js

window.onload = function () {
  if (typeof web3 !== 'undefined') {
    web3 = new Web3(web3.currentProvider);
  }

  var abi = [{ "inputs": [], "payable": false, "type": "constructor" }, { "anonymous": false, "inputs": [ { "indexed": false, "name": "x", "type": "uint256" } ], "name": "Foobar", "type": "event" } ];
  var address = "0x42fdca8935689fcdf2baedc707de6da978515564";
  var TestContract = web3.eth.contract(abi);
  var testInstance = TestContract.at(address);

  var foobarEvent = testInstance.Foobar(null, {fromBlock: 0, toBlock: 'latest'}, function (error, result) {
    if (error)
      console.error(error)
    else
      console.log("Got a foobar with " + result.args.x.toString());
  });
}

index.html

<!DOCTYPE html>
<html lang="en">
  <head></head>
  <body>
  <script src="web3.min.js"></script>
  <script src="app.js"></script>
  </body>
</html>

web3.version.api: "0.20.1"

Result (expected) with 3.10.0:

MetaMask - injected web3
Got a foobar with 1
Got a foobar with 2
Got a foobar with 3

result1

Result with 3.10.3-pre2:

<event emitter warnings>
MetaMask - injected web3

result2

Chromium version: Version 60.0.3112.113 (Developer Build) (64-bit)

testrpc version: EthereumJS TestRPC v4.1.1 (ganache-core: 1.1.2)

e00dan commented 5 years ago

EDIT

FOUND SOLUTION TO MY PROBLEM HERE:

https://github.com/MetaMask/metamask-extension/issues/5493#issuecomment-429419309

Original comment

I'm not sure if it's been fixed. I get the same error, just the number changed from 11 to 101. I think working with 101 contracts at once in dapp is not something unusual.

Error: Possible EventEmitter memory leak detected. 101 data listeners added. Use emitter.setMaxListeners() to increase limit

It's coming from Contract constructor, then packageInit and source seems to be setProvider:

 if(this.provider && this.provider.on) {
        this.provider.on('data', function requestManagerNotification(result, deprecatedResult){
            result = result || deprecatedResult; // this is for possible old providers, which may had the error first handler

            // check for result.method, to prevent old providers errors to pass as result
            if(result.method && _this.subscriptions[result.params.subscription] && _this.subscriptions[result.params.subscription].callback) {
                _this.subscriptions[result.params.subscription].callback(null, result.params.result);
            }
        });
        // TODO add error, end, timeout, connect??
        // this.provider.on('error', function requestManagerNotification(result){
        //     Object.keys(_this.subscriptions).forEach(function(id){
        //         if(_this.subscriptions[id].callback)
        //             _this.subscriptions[id].callback(err);
        //     });
        // }
    }

that is called from packageInit:

pkg._requestManager.setProvider(args[0], args[1]);

that is called from Contract constructor:

 // sets _requestmanager
    core.packageInit(this, [this.constructor.currentProvider]);

This means setProvider is called for each contract constructed? If yes, then it means it's not possible to call new Contract on one page more than 101 times?

W3stside commented 5 years ago

EDIT

FOUND SOLUTION TO MY PROBLEM HERE:

#5493 (comment)

Original comment

I'm not sure if it's been fixed. I get the same error, just the number changed from 11 to 101. I think working with 101 contracts at once in dapp is not something unusual.

Error: Possible EventEmitter memory leak detected. 101 data listeners added. Use emitter.setMaxListeners() to increase limit

It's coming from Contract constructor, then packageInit and source seems to be setProvider:

 if(this.provider && this.provider.on) {
        this.provider.on('data', function requestManagerNotification(result, deprecatedResult){
            result = result || deprecatedResult; // this is for possible old providers, which may had the error first handler

            // check for result.method, to prevent old providers errors to pass as result
            if(result.method && _this.subscriptions[result.params.subscription] && _this.subscriptions[result.params.subscription].callback) {
                _this.subscriptions[result.params.subscription].callback(null, result.params.result);
            }
        });
        // TODO add error, end, timeout, connect??
        // this.provider.on('error', function requestManagerNotification(result){
        //     Object.keys(_this.subscriptions).forEach(function(id){
        //         if(_this.subscriptions[id].callback)
        //             _this.subscriptions[id].callback(err);
        //     });
        // }
    }

that is called from packageInit:

pkg._requestManager.setProvider(args[0], args[1]);

that is called from Contract constructor:

 // sets _requestmanager
    core.packageInit(this, [this.constructor.currentProvider]);

This means setProvider is called for each contract constructed? If yes, then it means it's not possible to call new Contract on one page more than 101 times?

It is - it just warns you about a possible memory leak.