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

fix: Revert "remove MarkHghMemoryUsage api" #13421

Merged
merged 2 commits into from Jun 26, 2018

Conversation

Projects
None yet
6 participants
@deepak1556
Member

deepak1556 commented Jun 25, 2018

This reverts commit 0de85fd that went in accidentally with #12448. Part of the reason because we didn't have release branches in native_mate.

Fixes #12676
Ref #13102, #12864

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • commit messages or PR title follow semantic commit guidelines

@deepak1556 deepak1556 requested a review from electron/reviewers as a code owner Jun 25, 2018

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Jun 25, 2018

Contributor

Should we do the same in the 3-0-x branch?

Contributor

alexeykuzmin commented Jun 25, 2018

Should we do the same in the 3-0-x branch?

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Jun 25, 2018

Contributor

Part of the reason because we didn't have release branches in native_mate.

Can you please create one for the 2-0-x line?

Contributor

alexeykuzmin commented Jun 25, 2018

Part of the reason because we didn't have release branches in native_mate.

Can you please create one for the 2-0-x line?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Jun 25, 2018

Member

Should we do the same in the 3-0-x branch?

Nope, changes to the api were made starting with v8 6.4

Can you please create one for 2-0-x line?

Done https://github.com/electron/native-mate/commits/electron-2-0-x

Member

deepak1556 commented Jun 25, 2018

Should we do the same in the 3-0-x branch?

Nope, changes to the api were made starting with v8 6.4

Can you please create one for 2-0-x line?

Done https://github.com/electron/native-mate/commits/electron-2-0-x

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Jun 25, 2018

Contributor

Can you please create one for 2-0-x line?

Done https://github.com/electron/native-mate/commits/electron-2-0-x

@deepak1556
The branch now points to a commit electron/native-mate@99d9e26 while the vendor/native_mate submodule in the Electron's 2-0-x branch – to the electron/native-mate@894c96f

Are you sure that we can skip top three commits

894c96f (HEAD) Remove usage of MarkIndependent api
6a3d238 Merge pull request #21 from yzgyyang/master
31206cf Add support for FreeBSD
// 99d9e26 (origin/electron-2-0-x) Merge pull request #19 from electron/get-wrapper-const

in the 2-0-x branch?

Contributor

alexeykuzmin commented Jun 25, 2018

Can you please create one for 2-0-x line?

Done https://github.com/electron/native-mate/commits/electron-2-0-x

@deepak1556
The branch now points to a commit electron/native-mate@99d9e26 while the vendor/native_mate submodule in the Electron's 2-0-x branch – to the electron/native-mate@894c96f

Are you sure that we can skip top three commits

894c96f (HEAD) Remove usage of MarkIndependent api
6a3d238 Merge pull request #21 from yzgyyang/master
31206cf Add support for FreeBSD
// 99d9e26 (origin/electron-2-0-x) Merge pull request #19 from electron/get-wrapper-const

in the 2-0-x branch?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Jun 25, 2018

Member
894c96f (HEAD) Remove usage of MarkIndependent api

Should definitely not be included thats the commit this PR is trying to revert, as for the other two, they were added for FreeBSD support, but i didn't see a corresponding work in electron 2-0-x branch. I am open to adding them back.

6a3d238 Merge pull request #21 from yzgyyang/master
31206cf Add support for FreeBSD
// 99d9e26 (origin/electron-2-0-x) Merge pull request #19 from electron/get-wrapper-const
Member

deepak1556 commented Jun 25, 2018

894c96f (HEAD) Remove usage of MarkIndependent api

Should definitely not be included thats the commit this PR is trying to revert, as for the other two, they were added for FreeBSD support, but i didn't see a corresponding work in electron 2-0-x branch. I am open to adding them back.

6a3d238 Merge pull request #21 from yzgyyang/master
31206cf Add support for FreeBSD
// 99d9e26 (origin/electron-2-0-x) Merge pull request #19 from electron/get-wrapper-const
@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Jun 25, 2018

Contributor

@deepak1556 Let's just revert the changes that break stuff.
That FreeBSD support might be actually used by someone.

Contributor

alexeykuzmin commented Jun 25, 2018

@deepak1556 Let's just revert the changes that break stuff.
That FreeBSD support might be actually used by someone.

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Jun 25, 2018

Member

@deepak1556 I don't understand the issue and don't want to make the same mistake again. What went wrong with #12448 and what could have been done differently?

Member

ckerr commented Jun 25, 2018

@deepak1556 I don't understand the issue and don't want to make the same mistake again. What went wrong with #12448 and what could have been done differently?

@deepak1556 deepak1556 requested a review from zcbenz Jun 26, 2018

@zcbenz

zcbenz approved these changes Jun 26, 2018

👍

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Jun 26, 2018

Member

@ckerr the PR in native_mate was intended only master branch which used v8 6.4 and above. But since native_mate didn't have a 2-0 release branch, the change was accidentally pulled in which triggered the gc crashes (I should have been specific about which branches the original PR applied in its description, sorry about that). I am confident this won't happen again as we the got the branch setup in native_mate and also now that native_mate is part of electron since 3-0-x and above.

As for the api itself, we use MarkIndependent to destroy weak handles in first gc phase so that we get notification of gc and associated events emitted during destruction can be successful, currently api::WebContents relies on this.

Member

deepak1556 commented Jun 26, 2018

@ckerr the PR in native_mate was intended only master branch which used v8 6.4 and above. But since native_mate didn't have a 2-0 release branch, the change was accidentally pulled in which triggered the gc crashes (I should have been specific about which branches the original PR applied in its description, sorry about that). I am confident this won't happen again as we the got the branch setup in native_mate and also now that native_mate is part of electron since 3-0-x and above.

As for the api itself, we use MarkIndependent to destroy weak handles in first gc phase so that we get notification of gc and associated events emitted during destruction can be successful, currently api::WebContents relies on this.

@codebytere codebytere merged commit 4efed0f into 2-0-x Jun 26, 2018

11 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@alexeykuzmin alexeykuzmin deleted the revert_12448 branch Jun 26, 2018

@lsegal

This comment has been minimized.

Show comment
Hide comment
@lsegal

lsegal Jun 29, 2018

Contributor

Is this going to be released as a hotfix at some point soon? If not, are there any ideas how apps could workaround this in the meantime?

Contributor

lsegal commented Jun 29, 2018

Is this going to be released as a hotfix at some point soon? If not, are there any ideas how apps could workaround this in the meantime?

GitSquared added a commit to GitSquared/edex-ui that referenced this pull request Jul 5, 2018

Update electron to the latest version 🚀 (#115)

## Version **2.0.4** of **electron** was just published.

<table>
  <tr>
    <th align=left>
      Dependency
    </th>
    <td>
      <a target=_blank href=https://github.com/electron/electron>electron</a>
    </td>
  </tr>
  <tr>
      <th align=left>
       Current Version
      </th>
      <td>
        2.0.3
      </td>
    </tr>
  <tr>
    <th align=left>
      Type
    </th>
    <td>
      dependency
    </td>
  </tr>
</table>



The version **2.0.4** is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

It might be worth looking into these changes and trying to get this project onto the latest version of electron.

If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update.


---


<details>
<summary>Release Notes</summary>
<strong>electron v2.0.4</strong>

<h2>Bug Fixes</h2>
<ul>
<li>Fixed crashes in V8 garbage collector. <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="335502558" data-permission-text="Issue title is private" data-url="electron/electron#13421" href="https://urls.greenkeeper.io/electron/electron/pull/13421">#13421</a></li>
<li>Fixed issue where cookies and credentials were being captured in netlog. <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="337567636" data-permission-text="Issue title is private" data-url="electron/electron#13537" href="https://urls.greenkeeper.io/electron/electron/pull/13537">#13537</a></li>
</ul>
<h3>macOS</h3>
<ul>
<li>Fixed crash on startup on macOS 10.9 due to linking with CoreBluetooth. <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="337788399" data-permission-text="Issue title is private" data-url="electron/electron#13545" href="https://urls.greenkeeper.io/electron/electron/pull/13545">#13545</a></li>
<li>Fixed <code>tray.setContextMenu</code> crash. <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="337147553" data-permission-text="Issue title is private" data-url="electron/electron#13516" href="https://urls.greenkeeper.io/electron/electron/pull/13516">#13516</a></li>
</ul>
<h3>Windows</h3>
<ul>
<li>Fixed windows causing repaint issues when DWM composition is disabled. <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="310462684" data-permission-text="Issue title is private" data-url="electron/electron#12501" href="https://urls.greenkeeper.io/electron/electron/pull/12501">#12501</a></li>
</ul>
</details>


<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment