Issue with latest PhantomJS Master and "error.stack"

[Ivan De Marino]

This is mostly for Ariya and Jon, but I thought it would be better to discuss it here. Maybe others can pitch in.

Check out this change I currently made on a branch of mine: https://github.com/detro/phantomjs/commit/47430fed501e428596ade91bfe615db1f73af568#diff-2

I had to do that because, in my experience, if an error occurs in a Page (not the phantom.page, but a "normal" webpage), the "error" value at that point of the callback handling is undefined.

Can you guys confirm?

PS I think it's the "__exception" object within the Page frame that is not defined somehow.

PPS This is happening within GhostDriver, that does a fairly complex use of pages, contexts and so forth. Though, even in the worst intricated mix of Javascript, I think that "__exception" should still be set. Right?

[Jon Leighton]

Hey, are you able to provide a simple example? Then I can investigate.
Thanks

On 12/06/12 01:00, Ivan De Marino wrote:
> This is mostly for Ariya and Jon, but I thought it would be better to
> discuss it here. Maybe others can pitch in.
>
> Check out this change I currently made on a branch of mine:

> *https://github.com/detro/phantomjs/commit/47430fed501e428596ade91bfe615db1f73af568#diff-2*
>
> I had to do that because, in my experience, *if an error occurs in a
> Page* (not the phantom.page, but a "normal" webpage),*the "error" value
> at that point of the callback handling is undefined*.
>
> Can you guys confirm?
>
> PS I think it's the *"__exception" object within the Page frame that is
> not defined somehow*.

> PPS This is happening within GhostDriver, that does a fairly complex use
> of pages, contexts and so forth. Though, even in the worst intricated
> mix of Javascript, I think that "__exception" should still be set. Right?

--
http://jonathanleighton.com/

[Ivan De Marino]

It's pretty tricky, but I'll try to provide one by this evening.

The issue appears when loading a WebDriver Atoms chunk of JS: it's a freaking massive amount of JS to load as string via "evaluate".

Stay tuned and thanks :)

--

Ivan De Marino

Coder, Technologist, Cook, Italian

blog.ivandemarino.me | www.linkedin.com/in/ivandemarino | twitter.com/detronizator

[david]

Could this possibly be related ?

http://code.google.com/p/phantomjs/issues/detail?id=589 

I got a hard to reproduce error in PhantomJS : the "error" object in the error handler was "null" in bootstrap.js.

Among other problems, "typeof null === object" is true, so bootstrap.js was failing at line 89.

I made a patch (quick fix to avoid PhantomJS from failing completely) and sent a pull request : 

https://github.com/ariya/phantomjs/pull/266/files 

David

[Ivan De Marino]

Yep, that's the issue I got.

We need to give Jon a script to launch and reproduce.

Have you got one?

[Jon Leighton]

Hey,

I can't reproduce with the following example:
https://gist.github.com/2925648

Can you help me reproduce the issue please?

Thanks

On 12/06/12 01:00, Ivan De Marino wrote:

> This is mostly for Ariya and Jon, but I thought it would be better to
> discuss it here. Maybe others can pitch in.
>
> Check out this change I currently made on a branch of mine:

> *https://github.com/detro/phantomjs/commit/47430fed501e428596ade91bfe615db1f73af568#diff-2*
>
> I had to do that because, in my experience, *if an error occurs in a
> Page* (not the phantom.page, but a "normal" webpage),*the "error" value
> at that point of the callback handling is undefined*.
>
> Can you guys confirm?
>
> PS I think it's the *"__exception" object within the Page frame that is
> not defined somehow*.

> PPS This is happening within GhostDriver, that does a fairly complex use
> of pages, contexts and so forth. Though, even in the worst intricated
> mix of Javascript, I think that "__exception" should still be set. Right?

--
http://jonathanleighton.com/

[Jon Leighton]

Sorry, I sent this before I saw your and David's replies. Didn't mean to
be impatient :)

[Jon Leighton]

Ok, I have been able to reproduce a problem when an error occurs within
a sub-frame of the page. I'll work up a fix and you guys can let me know
if it solves your issues.

[david]

I was working on a script to reproduce the error (without much success) but it seems that Jon was able to reproduce the bug :) Let us know Jon !

Also, if a fix can't be sorted out before 1.6, since this is a serious show-stopper, what do you think about merging this pull request ?

https://github.com/ariya/phantomjs/pull/266/files  

Thanks !

On Wednesday, June 13, 2012 3:12:51 PM UTC+2, Ivan De Marino wrote:

Yep, that's the issue I got.

We need to give Jon a script to launch and reproduce.

Have you got one?

[Ivan De Marino]

There you go: https://gist.github.com/2929203

This should show all the "weird" errors I'm getting.

Particularly, I think the evaluation of code "via string" is broken. Badly.

[Jon Leighton]

Thanks, that's useful. I will take a look but I am not sure the string
issue is related to the error handling. If you wrap your strings in a
function it will work, e.g.

p.evaluate("function() { console.log('first message'); }");

Without the function wrapper it generates a parse error for some sort of
internal reason that I haven't investigated yet (and we don't do stack
traces for parse errors)

Jon

On 14/06/12 10:07, Ivan De Marino wrote:
> There you go: https://gist.github.com/2929203
>
> This should show all the "weird" errors I'm getting.
>
> Particularly, I think the evaluation of code "via string" is broken. Badly.
>
> On 13 June 2012 20:10, Jon Leighton <j...@jonathanleighton.com

> <mailto:j...@jonathanleighton.com>> wrote:
>
> Ok, I have been able to reproduce a problem when an error occurs
> within a sub-frame of the page. I'll work up a fix and you guys can
> let me know if it solves your issues.
>
>
> On 13/06/12 20:03, Jon Leighton wrote:
>
> Sorry, I sent this before I saw your and David's replies. Didn't
> mean to
> be impatient :)
>
> On 13/06/12 19:26, Jon Leighton wrote:
>
> Hey,
>
> I can't reproduce with the following example:

> https://gist.github.com/__2925648

> <https://gist.github.com/2925648>
>
> Can you help me reproduce the issue please?
>
> Thanks
>
> On 12/06/12 01:00, Ivan De Marino wrote:
>
> This is mostly for Ariya and Jon, but I thought it would
> be better to
> discuss it here. Maybe others can pitch in.
>
> Check out this change I currently made on a branch of mine:

> *https://github.com/detro/__phantomjs/commit/__47430fed501e428596ade91bfe615d__b1f73af568#diff-2*

> <https://github.com/detro/phantomjs/commit/47430fed501e428596ade91bfe615db1f73af568#diff-2*>
>
>
>
> I had to do that because, in my experience, *if an error
> occurs in a
> Page* (not the phantom.page, but a "normal"
> webpage),*the "error" value
> at that point of the callback handling is undefined*.
>
> Can you guys confirm?
>
> PS I think it's the *"__exception" object within the
> Page frame that is
> not defined somehow*.
> PPS This is happening within GhostDriver, that does a
> fairly complex use
> of pages, contexts and so forth. Though, even in the
> worst intricated
> mix of Javascript, I think that "__exception" should
> still be set.
> Right?
>
>
>
>
> --
> http://jonathanleighton.com/
>
>
>
>
> --

> *Ivan De Marino*
> Coder, Technologist, Cook, Italian
>
> blog.ivandemarino.me <http://blog.ivandemarino.me> |
> www.linkedin.com/in/ivandemarino
> <http://www.linkedin.com/in/ivandemarino> | twitter.com/detronizator
> <http://twitter.com/detronizator>

--
http://jonathanleighton.com/

[Ivan De Marino]

Well, the key point for us is the absence of stack trace causing the issue.

We can leave without it, but we need to be more "defensive".

Probably the patch David is proposing is enough.

Maybe it should consider that a message might be there, but no "stack".

--

Ivan De Marino

Coder, Technologist, Cook, Italian

blog.ivandemarino.me | www.linkedin.com/in/ivandemarino | twitter.com/detronizator

[Ariya Hidayat]

> Also, if a fix can't be sorted out before 1.6, since this is a serious
> show-stopper, what do you think about merging this pull request ?
> https://github.com/ariya/phantomjs/pull/266/files

I kind of agree with this. Even if the problem will be fixed later,
the above guard is still useful.

I'll merge it unless there is a strong objection.




--
Ariya Hidayat, http://ariya.ofilabs.com
http://twitter.com/ariyahidayat

[Ivan De Marino]

Go for it.

[Jon Leighton]

I changed the implementation, which I think should be more stable. See
https://github.com/ariya/phantomjs/pull/270

Jon

On 15/06/12 07:52, Ivan De Marino wrote:
> Go for it.
>
> On 15 June 2012 02:03, Ariya Hidayat <ariya....@gmail.com

> <mailto:ariya....@gmail.com>> wrote:
>
> > Also, if a fix can't be sorted out before 1.6, since this is a
> serious
> > show-stopper, what do you think about merging this pull request ?
> > https://github.com/ariya/phantomjs/pull/266/files
>
> I kind of agree with this. Even if the problem will be fixed later,
> the above guard is still useful.
>
> I'll merge it unless there is a strong objection.
>
>
>
>
> --
> Ariya Hidayat, http://ariya.ofilabs.com
> http://twitter.com/ariyahidayat
>
>
>
>
> --

> *Ivan De Marino*
> Coder, Technologist, Cook, Italian
>