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

Pack URL bug #37

Closed
o2genum opened this issue Nov 16, 2017 · 12 comments
Closed

Pack URL bug #37

o2genum opened this issue Nov 16, 2017 · 12 comments

Comments

@o2genum
Copy link

o2genum commented Nov 16, 2017

The new pack:/// url feature seems to be buggy.

From time to time Chromium is unable to read random packed files (I run the same executable each time, without rebuilding, and sometimes it happens). For big files it happens more frequently. Add something like 30K-line javascript to reproduce it. Unexpected end of input, unexpected token, etc., each time on a random line.

capture

If I click build.js:1 to go to that line, dev tools close. The affected files are missing Sources tab. It I click Application tab, dev tools close.

Maybe the contributor @lhyqy5 knows what's wrong.

@David-Desmaisons
Copy link
Member

Could you share the corresponding project/file?

@o2genum
Copy link
Author

o2genum commented Nov 16, 2017

I'll make up a sample project to reproduce it.

@o2genum
Copy link
Author

o2genum commented Nov 16, 2017

Here it is. Just a clean solution template + added a heavy main.js.

Open chromium dev tools, about every second run, it logs an error to the console.

https://github.com/o2genum/NeutroniumPackUriExample

@o2genum
Copy link
Author

o2genum commented Nov 16, 2017

Must be related to the copying of data between the streams: PackUriResourceHandler.cs

Related ChromoumFX docs.

@David-Desmaisons
Copy link
Member

@o2genum , I just corrected it. It was one nasty issue due to the way of ChromiumFx manage memory (using weak references) that may lead to hard to reproduce bug.

@o2genum
Copy link
Author

o2genum commented Nov 17, 2017

Ok. I suspected the copying of data in PackUriResourceHandler.cs and it is coded a bit awkwardly, indeed. It only works because ResponseLength is set.

I corrected it according to the ChromiumFX docs (the code below), maybe you can include this fix, too. This is unrelated to the issue, though.

Will you make a release for this fix?

            ReadResponse += (s2, e2) =>
            {
                if (resInfo != null)
                {
                    var buffer = new byte[e2.BytesToRead];
                    var bytesRead = resInfo.Stream.Read(buffer, 0, e2.BytesToRead);
                    System.Runtime.InteropServices.Marshal.Copy(buffer, 0, e2.DataOut, bytesRead);
                    e2.BytesRead = bytesRead;

                    if (bytesRead != 0)
                    {
                        e2.SetReturnValue(true);
                    }
                    else
                    {
                        e2.SetReturnValue(false);
                    }
                }
            };

            ProcessRequest += (s3, e3) =>
            {
                e3.SetReturnValue(true);
                e3.Callback.Continue();
            };

@David-Desmaisons
Copy link
Member

Are you refering to this alteration:

   if (bytesRead != 0)
   {
         e2.SetReturnValue(true);
   }
  else
   {
         e2.SetReturnValue(false);
   }

?

The new version should be available shortly.

Could you comunicate what kind of application you are creating with Neutronium?
Would you be ok to share some screenshots on Neutronium documentation to ilustre aplication made with Neutronium?

@o2genum
Copy link
Author

o2genum commented Nov 17, 2017

This alteration, also unnecessary Continue() in ReadResponce, also wrong order (Continue(), then SetReturnValue()) in ReadRequest and ProcessRequest.

Ok, I'll share the app description and screenshots when I got it working (we're in process of switching to Neutronium-based UI).

@David-Desmaisons
Copy link
Member

Ok Thanks.
I will take a look at the alteration, and commit them after review.

@o2genum
Copy link
Author

o2genum commented Nov 18, 2017

Ok. I only looked at what the docs say.

e.SetReturnValue(false) is for signalling end of input, but when e.ResponseLength is set, this is not necessary (Chromium stops reading before the end of input is met). So I think it's ok as it is.

@David-Desmaisons
Copy link
Member

Thanks for the link. I made another alteration to the original base on this doc.

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

No branches or pull requests

2 participants