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

Fix broken links generated from Node modules in DevTools #4

Open
wants to merge 1 commit into
base: nw13
Choose a base branch
from

Conversation

ghostoy
Copy link
Member

@ghostoy ghostoy commented Mar 28, 2016

Compiled scripts from Node were set to weak, which caused the script
object in V8 being GCed shortly. Hence links of the script shown in
console of DevTools become invalid after GC. This patch saved compiled
scripts from Node globally to survival from GC.

Fixed nwjs/nw.js#4269

@rogerwang
Copy link
Member

Is this PR the only one needed to fix 4269?

@ghostoy
Copy link
Member Author

ghostoy commented Mar 28, 2016

@rogerwang I just updated the patch. The idea is similar as previous patch, but moved the implementation from C++ to JS. And it also fixes the broken links in app window by transforming filename into file:// protocol. Adding file:// protocol will break libraries, like bindings, which detects calling script from stack trace. So I've removed this part from patch.

And yes, it's the only patch to fix the issue. I'll submit test case to nw.js repo later.

@ghostoy ghostoy changed the title Unset weak status of compiled scripts from Node Fix broken links generated from Node modules in DevTools Mar 28, 2016
@rogerwang rogerwang force-pushed the nw13 branch 4 times, most recently from 404e0c2 to f1993fc Compare April 3, 2016 11:58
Compiled scripts from Node were set to weak, which caused the script
object in V8 being GCed shortly. Hence links of the script shown in
console of DevTools become invalid after GC. This patch saved compiled
scripts from Node globally to survival from GC.

Fixed nwjs/nw.js#4269
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.

2 participants