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

PhantomJs: ??? in onError + JSCom has been closed #1555

Closed
japgolly opened this Issue Mar 19, 2015 · 32 comments

Comments

Projects
None yet
3 participants
@japgolly
Contributor

japgolly commented Mar 19, 2015

Reproducies:

git clone https://github.com/japgolly/scalacss.git
cd scalacss
git checkout aed8b40
sbt core-js/test

It's been failing ever since I switched jsEnv to phantom js.

I get (abbreviated):

  /tmp/phantomjs-launcher5284409069744574786.js:9 in onError
  /tmp/phantomjs-launcher5284409069744574786.js:11 in onError
  file:///home/golly/projects/scalacss/core/target/js/scala-2.11/core-test-fastopt.js:79391 (in function "doWriteLine__p4__T__V")
  /tmp/phantomjs-launcher5284409069744574786.js:13
[trace] Stack trace suppressed: run last core-js/test:test for the full output.
[error] Could not run test japgolly.scalacss.DefaultsTest: org.scalajs.jsenv.ComJSEnv$ComClosedException: JSCom has been closed

When I run last core-js/test:test I get:

[debug] Running TaskDef(japgolly.scalacss.mutable.RegisterTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@6cd53313, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.UsageTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@11aa802b, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.full.StandaloneTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@6d90e4, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.DefaultsTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@1eb0487b, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.CondTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@206986cc, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.AttrTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@389590e0, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.ValueTTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@aa44995, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.StyleTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@531db443, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.full.InlineTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@51993e4a, false, [SuiteSelector])
[debug] Running TaskDef(japgolly.scalacss.ComposeTest, org.scalajs.testadapter.FingerprintSerializers$DeserializedSubclassFingerprint@16430dbf, false, [SuiteSelector])
org.scalajs.jsenv.ComJSEnv$ComClosedException: JSCom has been closed
    at org.scalajs.jsenv.phantomjs.PhantomJSEnv$ComPhantomRunner.receiveFrag(PhantomJSEnv.scala:287)
    at org.scalajs.jsenv.phantomjs.PhantomJSEnv$ComPhantomRunner.loop$1(PhantomJSEnv.scala:258)
    at org.scalajs.jsenv.phantomjs.PhantomJSEnv$ComPhantomRunner.receive(PhantomJSEnv.scala:273)
    at org.scalajs.testadapter.ComUtils$.receiveResponse(ComUtils.scala:42)
    at org.scalajs.testadapter.ComUtils$.receiveLoop(ComUtils.scala:21)
    at org.scalajs.testadapter.ComUtils$.receiveLoop(ComUtils.scala:16)
    at org.scalajs.testadapter.ScalaJSTask.execute(ScalaJSTask.scala:62)
    at sbt.TestRunner.runTest$1(TestFramework.scala:76)
    at sbt.TestRunner.run(TestFramework.scala:85)
    at sbt.TestFramework$$anon$2$$anonfun$$init$$1$$anonfun$apply$8.apply(TestFramework.scala:202)
    at sbt.TestFramework$$anon$2$$anonfun$$init$$1$$anonfun$apply$8.apply(TestFramework.scala:202)
    at sbt.TestFramework$.sbt$TestFramework$$withContextLoader(TestFramework.scala:185)
    at sbt.TestFramework$$anon$2$$anonfun$$init$$1.apply(TestFramework.scala:202)
    at sbt.TestFramework$$anon$2$$anonfun$$init$$1.apply(TestFramework.scala:202)
    at sbt.TestFunction.apply(TestFramework.scala:207)
    at sbt.Tests$$anonfun$9.apply(Tests.scala:216)
    at sbt.Tests$$anonfun$9.apply(Tests.scala:216)
    at sbt.std.Transform$$anon$3$$anonfun$apply$2.apply(System.scala:44)
    at sbt.std.Transform$$anon$3$$anonfun$apply$2.apply(System.scala:44)
    at sbt.std.Transform$$anon$4.work(System.scala:63)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
    at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
    at sbt.Execute.work(Execute.scala:235)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
    at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
    at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

@japgolly japgolly changed the title from ComClosedException: JSCom has been closed to PhantomJs: ??? in onError + JSCom has been closed Mar 19, 2015

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Mar 19, 2015

Member

Thanks for the report. I'll probably not be able to look into it before 26th, though.

Member

sjrd commented Mar 19, 2015

Thanks for the report. I'll probably not be able to look into it before 26th, though.

@sjrd sjrd added the bug label Mar 19, 2015

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Mar 19, 2015

Contributor

np :)

I'll lock in a commit in the reproduction instructions above and switch off phantomjs for now.

Contributor

japgolly commented Mar 19, 2015

np :)

I'll lock in a commit in the reproduction instructions above and switch off phantomjs for now.

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Mar 19, 2015

Contributor

Sweet!! Finally we can reproduce this.

Contributor

gzm0 commented Mar 19, 2015

Sweet!! Finally we can reproduce this.

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Mar 20, 2015

Contributor

I can't reproduce this :( It seems this is really just a race somewhere...

Contributor

gzm0 commented Mar 20, 2015

I can't reproduce this :( It seems this is really just a race somewhere...

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Mar 20, 2015

Contributor

Oh no really? You definately checked it out to aed8b40 (cos I switched to Node JS after that commit). For me it happens 100% of the time. Maybe there's an environmental factor.​..

> uname -a
Linux golly-desktop 3.18.6-1-ARCH #1 SMP PREEMPT Sat Feb 7 08:44:05 CET 2015 x86_64 GNU/Linux

> phantomjs -v      
2.0.0

> java -version
java version "1.8.0_40"
Java(TM) SE Runtime Environment (build 1.8.0_40-b25)
Java HotSpot(TM) 64-Bit Server VM (build 25.40-b25, mixed mode)
Contributor

japgolly commented Mar 20, 2015

Oh no really? You definately checked it out to aed8b40 (cos I switched to Node JS after that commit). For me it happens 100% of the time. Maybe there's an environmental factor.​..

> uname -a
Linux golly-desktop 3.18.6-1-ARCH #1 SMP PREEMPT Sat Feb 7 08:44:05 CET 2015 x86_64 GNU/Linux

> phantomjs -v      
2.0.0

> java -version
java version "1.8.0_40"
Java(TM) SE Runtime Environment (build 1.8.0_40-b25)
Java HotSpot(TM) 64-Bit Server VM (build 25.40-b25, mixed mode)
@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Mar 20, 2015

Contributor
$ phantomjs -v
1.9.0

That might be it... :S

Contributor

gzm0 commented Mar 20, 2015

$ phantomjs -v
1.9.0

That might be it... :S

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 2, 2015

Contributor

I've just started getting this in a completely different (private) repo. :(

Contributor

japgolly commented Apr 2, 2015

I've just started getting this in a completely different (private) repo. :(

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 2, 2015

Contributor

This has something to do with object initialisation order I believe.
Changing val to def or lazy val in one of my objects has made this go away. But really, I can't see anything even close to a cyclic dependency or anything. In fact, the object I mentioned is a new one I created an hour ago that only 2 tests reference.

Contributor

japgolly commented Apr 2, 2015

This has something to do with object initialisation order I believe.
Changing val to def or lazy val in one of my objects has made this go away. But really, I can't see anything even close to a cyclic dependency or anything. In fact, the object I mentioned is a new one I created an hour ago that only 2 tests reference.

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 20, 2015

Contributor

Is there anyway we can prioritise this? This issue follows me around from project to project.

Contributor

japgolly commented Apr 20, 2015

Is there anyway we can prioritise this? This issue follows me around from project to project.

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 20, 2015

Contributor

It always mentions doWriteLine__p4__T__V. Is there any chance this doWriteLine fn isn't available in PhantomJS 2.0 or something like that?

  /tmp/phantomjs-launcher3933850603413980206.js:11 in onError
  file:///.../webapp-client-test-fastopt.js:244379 (in function "doWriteLine__p4__T__V")
Contributor

japgolly commented Apr 20, 2015

It always mentions doWriteLine__p4__T__V. Is there any chance this doWriteLine fn isn't available in PhantomJS 2.0 or something like that?

  /tmp/phantomjs-launcher3933850603413980206.js:11 in onError
  file:///.../webapp-client-test-fastopt.js:244379 (in function "doWriteLine__p4__T__V")
@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 20, 2015

Contributor

This makes me want to swear. A lot.
ariya/phantomjs#13112

PhantomJS releases are very few and far between... Could we pretty-please have a workaround in SJS for this? Like a configuration setting that determines which console.xxx method to use?

Contributor

japgolly commented Apr 20, 2015

This makes me want to swear. A lot.
ariya/phantomjs#13112

PhantomJS releases are very few and far between... Could we pretty-please have a workaround in SJS for this? Like a configuration setting that determines which console.xxx method to use?

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 20, 2015

Member

If you can come up with a work-around in doWriteLine that still uses console.error on other environments, I'm happy to take it. A configuration setting is unlikely to work, since we can't change the code of doWriteLine from a setting.

In any case I'll schedule a least a work-around for 0.6.3.

Member

sjrd commented Apr 20, 2015

If you can come up with a work-around in doWriteLine that still uses console.error on other environments, I'm happy to take it. A configuration setting is unlikely to work, since we can't change the code of doWriteLine from a setting.

In any case I'll schedule a least a work-around for 0.6.3.

@sjrd sjrd added this to the v0.6.3 milestone Apr 20, 2015

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 20, 2015

Contributor

I wonder what we could do. Thinking slightly bigger, it might be handy if developers were able to control the behaviour of System.{err,out} in their Scala-JS apps... In addition to allowing devs to solve this specific problem themselves (still a burden though), it will also allow them to do things like mute noisy libraries, and replace stderr (in prod) with an ajax call that sends error messages back to the server.

What do you think about System.{err,out} delegating to some globals that are initialised with defaults? (eg. The value of System.out is in a JS global ScalaJS.System.out distinguishable from java.lang.System.out)

Contributor

japgolly commented Apr 20, 2015

I wonder what we could do. Thinking slightly bigger, it might be handy if developers were able to control the behaviour of System.{err,out} in their Scala-JS apps... In addition to allowing devs to solve this specific problem themselves (still a burden though), it will also allow them to do things like mute noisy libraries, and replace stderr (in prod) with an ajax call that sends error messages back to the server.

What do you think about System.{err,out} delegating to some globals that are initialised with defaults? (eg. The value of System.out is in a JS global ScalaJS.System.out distinguishable from java.lang.System.out)

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 20, 2015

Member

You can mutate java.lang.System.out if you want.

Member

sjrd commented Apr 20, 2015

You can mutate java.lang.System.out if you want.

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 20, 2015

Contributor

How? System.setErr(System.out) causes Referring to non-existent method jl_System$.setErr__Ljava_io_PrintStream__V.

Contributor

japgolly commented Apr 20, 2015

How? System.setErr(System.out) causes Referring to non-existent method jl_System$.setErr__Ljava_io_PrintStream__V.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 20, 2015

Member

Ah, right. Well I guess we need to add those. :-s

Member

sjrd commented Apr 20, 2015

Ah, right. Well I guess we need to add those. :-s

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 20, 2015

Contributor

Ah, I thought I was missing something again. :) Raised #1598

So the above will facilitate users being able to workaround it themselves (everywhere in source). What should we do to prevent such a need? Maybe redirect System.err to out for all PhantomJS envs? Is there a setting similar to initialCommands in Console but for JSEnvs? If so, users could add the redirection once in SBT to have it executed on env startup before client code is run.

Contributor

japgolly commented Apr 20, 2015

Ah, I thought I was missing something again. :) Raised #1598

So the above will facilitate users being able to workaround it themselves (everywhere in source). What should we do to prevent such a need? Maybe redirect System.err to out for all PhantomJS envs? Is there a setting similar to initialCommands in Console but for JSEnvs? If so, users could add the redirection once in SBT to have it executed on env startup before client code is run.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 20, 2015

Member

You can subclass JSEnv classes. I think they all have an overridable method that corresponds to this. For PhantomJS that would be https://github.com/scala-js/scala-js/blob/master/js-envs/src/main/scala/org/scalajs/jsenv/ExternalJSEnv.scala#L30

Member

sjrd commented Apr 20, 2015

You can subclass JSEnv classes. I think they all have an overridable method that corresponds to this. For PhantomJS that would be https://github.com/scala-js/scala-js/blob/master/js-envs/src/main/scala/org/scalajs/jsenv/ExternalJSEnv.scala#L30

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Apr 20, 2015

Contributor

Indeed. Actually, I think we should rather solve this particular problem on a JS level (so the Scala code remains as VM agnostic as possibled). Stderr will automatically be redirected to console.log, if console.error does not exist. Therefore, it should be sufficient to do this:

class PhantomJS2Env(
    phantomjsPath: String = "phantomjs",
    addArgs: Seq[String] = Seq.empty,
    addEnv: Map[String, String] = Map.empty,
    val autoExit: Boolean = true,
    jettyClassLoader: ClassLoader = null
) extends PhantomJSEnv(phantomjsPath, addArgs, addEnv, autoExit, jettyClassLoader) {

  override protected def vmName: String = "PhantomJS2"

  private val consoleNuker = new MemVirtualJSFile("consoleNuker.js")
      .withContent("console.error = undefined;")

  override def jsRunner(classpath: CompleteClasspath, code: VirtualJSFile,
      logger: Logger, console: JSConsole): JSRunner = {
    new PhantomRunner(classpath, code, logger, console) {
      override protected def initFiles(): Seq[VirtualJSFile] = super.initFiles :+ consoleNuker
    }
  }

  override def asyncRunner(classpath: CompleteClasspath, code: VirtualJSFile,
      logger: Logger, console: JSConsole): AsyncJSRunner = {
    new AsyncPhantomRunner(classpath, code, logger, console) {
      override protected def initFiles(): Seq[VirtualJSFile] = super.initFiles :+ consoleNuker
    }
  }

  override def comRunner(classpath: CompleteClasspath, code: VirtualJSFile,
      logger: Logger, console: JSConsole): ComJSRunner = {
    new ComPhantomRunner(classpath, code, logger, console) {
      override protected def initFiles(): Seq[VirtualJSFile] = super.initFiles :+ consoleNuker
    }
  }
}

And then use this, instead of the standard PhantomJS:

// Set other stuff as you require
postLinkJSEnv := new PhantomJS2Env(jettyClassLoader = scalaJSPhantomJSClassLoader.value)
Contributor

gzm0 commented Apr 20, 2015

Indeed. Actually, I think we should rather solve this particular problem on a JS level (so the Scala code remains as VM agnostic as possibled). Stderr will automatically be redirected to console.log, if console.error does not exist. Therefore, it should be sufficient to do this:

class PhantomJS2Env(
    phantomjsPath: String = "phantomjs",
    addArgs: Seq[String] = Seq.empty,
    addEnv: Map[String, String] = Map.empty,
    val autoExit: Boolean = true,
    jettyClassLoader: ClassLoader = null
) extends PhantomJSEnv(phantomjsPath, addArgs, addEnv, autoExit, jettyClassLoader) {

  override protected def vmName: String = "PhantomJS2"

  private val consoleNuker = new MemVirtualJSFile("consoleNuker.js")
      .withContent("console.error = undefined;")

  override def jsRunner(classpath: CompleteClasspath, code: VirtualJSFile,
      logger: Logger, console: JSConsole): JSRunner = {
    new PhantomRunner(classpath, code, logger, console) {
      override protected def initFiles(): Seq[VirtualJSFile] = super.initFiles :+ consoleNuker
    }
  }

  override def asyncRunner(classpath: CompleteClasspath, code: VirtualJSFile,
      logger: Logger, console: JSConsole): AsyncJSRunner = {
    new AsyncPhantomRunner(classpath, code, logger, console) {
      override protected def initFiles(): Seq[VirtualJSFile] = super.initFiles :+ consoleNuker
    }
  }

  override def comRunner(classpath: CompleteClasspath, code: VirtualJSFile,
      logger: Logger, console: JSConsole): ComJSRunner = {
    new ComPhantomRunner(classpath, code, logger, console) {
      override protected def initFiles(): Seq[VirtualJSFile] = super.initFiles :+ consoleNuker
    }
  }
}

And then use this, instead of the standard PhantomJS:

// Set other stuff as you require
postLinkJSEnv := new PhantomJS2Env(jettyClassLoader = scalaJSPhantomJSClassLoader.value)
@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 21, 2015

Contributor

That looks like it'd do the job. Is this something you envision being part of Scala.JS? It's quite a large snippet :)

Also that's a lot of boilerplate just to add some initial-JS. It might be worthwhile to add an initFiles: Seq[VirtualJSFile] arg to constructors of all JSenv classes, that appends it to each runner.

Contributor

japgolly commented Apr 21, 2015

That looks like it'd do the job. Is this something you envision being part of Scala.JS? It's quite a large snippet :)

Also that's a lot of boilerplate just to add some initial-JS. It might be worthwhile to add an initFiles: Seq[VirtualJSFile] arg to constructors of all JSenv classes, that appends it to each runner.

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Apr 21, 2015

Contributor

I don't know if this should be part of Scala.js. Are you working with PhantomJS 2.0 or with a pre-release version? Maybe we should upgrade to PhantomJS 2.0 anyways internally...

@sjrd what do you think? That would also give us proper websockets, meaning we could upgrade to Jetty9.

Contributor

gzm0 commented Apr 21, 2015

I don't know if this should be part of Scala.js. Are you working with PhantomJS 2.0 or with a pre-release version? Maybe we should upgrade to PhantomJS 2.0 anyways internally...

@sjrd what do you think? That would also give us proper websockets, meaning we could upgrade to Jetty9.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Apr 21, 2015

Member

Upgrading to PhantomJS 2.0 would be nice, but isn't it a bit too early to drop support for PhantomJS 1.x?
I'd sooner drop support for JDK 6 ...

Member

sjrd commented Apr 21, 2015

Upgrading to PhantomJS 2.0 would be nice, but isn't it a bit too early to drop support for PhantomJS 1.x?
I'd sooner drop support for JDK 6 ...

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Apr 22, 2015

Contributor

It's PhantomJS 2.0 final. The issue is marked as a regression so it must be a problem in certain version of PhantomJS 1.x as well.

Contributor

japgolly commented Apr 22, 2015

It's PhantomJS 2.0 final. The issue is marked as a regression so it must be a problem in certain version of PhantomJS 1.x as well.

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Apr 22, 2015

Contributor

Is the error we get from PhantomJS catchable? If yes, we can easily monkey patch console.error to handle the failure gracefully. Otherwise we'll need to:

  • Distinguish different PhantomJS versions
  • Never user console.error when in PhantomJS

In any case, this will be a testing nightmare for us, since we'll have to test with multiple versions of PhantomJS :(

Contributor

gzm0 commented Apr 22, 2015

Is the error we get from PhantomJS catchable? If yes, we can easily monkey patch console.error to handle the failure gracefully. Otherwise we'll need to:

  • Distinguish different PhantomJS versions
  • Never user console.error when in PhantomJS

In any case, this will be a testing nightmare for us, since we'll have to test with multiple versions of PhantomJS :(

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd May 1, 2015

Member

What do we want to do about this? It's definitely a crasher in PhantomJS, so it must eventually be fixed.

Maybe we could add an overrideable method customInitFiles() in ExternalJSEnv, and have AbstractExtRunner.initFiles() call its enclosing JSEnv's customInitFiles() instead of returning an empty seq. That would reduce significantly the ceremony of the above workaround:

class PhantomJS2Env(
    phantomjsPath: String = "phantomjs",
    addArgs: Seq[String] = Seq.empty,
    addEnv: Map[String, String] = Map.empty,
    val autoExit: Boolean = true,
    jettyClassLoader: ClassLoader = null
) extends PhantomJSEnv(phantomjsPath, addArgs, addEnv, autoExit, jettyClassLoader) {

  override protected def vmName: String = "PhantomJS2"

  private val consoleNuker = new MemVirtualJSFile("consoleNuker.js")
      .withContent("console.error = undefined;")

  override protected def customInitFiles(): Seq[VirtualJSFile] =
    super.customInitFiles() :+ consoleNuker
}

@japgolly Would that be acceptable until they fix the issue in PhantomJS?

Should we provide PhantomJS2EnvWithConsoleErrorWorkaround defined above instead?

Member

sjrd commented May 1, 2015

What do we want to do about this? It's definitely a crasher in PhantomJS, so it must eventually be fixed.

Maybe we could add an overrideable method customInitFiles() in ExternalJSEnv, and have AbstractExtRunner.initFiles() call its enclosing JSEnv's customInitFiles() instead of returning an empty seq. That would reduce significantly the ceremony of the above workaround:

class PhantomJS2Env(
    phantomjsPath: String = "phantomjs",
    addArgs: Seq[String] = Seq.empty,
    addEnv: Map[String, String] = Map.empty,
    val autoExit: Boolean = true,
    jettyClassLoader: ClassLoader = null
) extends PhantomJSEnv(phantomjsPath, addArgs, addEnv, autoExit, jettyClassLoader) {

  override protected def vmName: String = "PhantomJS2"

  private val consoleNuker = new MemVirtualJSFile("consoleNuker.js")
      .withContent("console.error = undefined;")

  override protected def customInitFiles(): Seq[VirtualJSFile] =
    super.customInitFiles() :+ consoleNuker
}

@japgolly Would that be acceptable until they fix the issue in PhantomJS?

Should we provide PhantomJS2EnvWithConsoleErrorWorkaround defined above instead?

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 May 1, 2015

Contributor

Should we provide PhantomJS2EnvWithConsoleErrorWorkaround defined above instead?

I don't think we should. In the long run, we should provide proper support for PhantomJS 2.0, with RFC 6455 websockets and Jetty9. So we might will have to envision to create a new separate environment. In the meantime, adding customInitFiles is probably the way to go.

Contributor

gzm0 commented May 1, 2015

Should we provide PhantomJS2EnvWithConsoleErrorWorkaround defined above instead?

I don't think we should. In the long run, we should provide proper support for PhantomJS 2.0, with RFC 6455 websockets and Jetty9. So we might will have to envision to create a new separate environment. In the meantime, adding customInitFiles is probably the way to go.

@sjrd sjrd self-assigned this May 1, 2015

sjrd added a commit to sjrd/scala-js that referenced this issue May 1, 2015

Add a customInitFiles() hook in ExternalJSEnv.
This will simplify the user-space workaround for scala-js#1555. It can also
be generally useful.

sjrd added a commit to sjrd/scala-js that referenced this issue May 1, 2015

Add a customInitFiles() hook in ExternalJSEnv.
This will simplify the user-space workaround for scala-js#1555. It can also
be generally useful.
@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 May 1, 2015

Contributor

@sjrd after #1630 is in, I think we should close this and open the a more general PhantomJS 2.0 support ticket.

Contributor

gzm0 commented May 1, 2015

@sjrd after #1630 is in, I think we should close this and open the a more general PhantomJS 2.0 support ticket.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd May 1, 2015

Member

I would leave this one open. After all it's not solved. We can cross-reference this ticket from the more general PhantomJS 2.0 one.

Member

sjrd commented May 1, 2015

I would leave this one open. After all it's not solved. We can cross-reference this ticket from the more general PhantomJS 2.0 one.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd May 1, 2015

Member

OK customInitFiles() is in. I'll keep this open, as it's not actually solved, but remove it from v0.6.3.

Member

sjrd commented May 1, 2015

OK customInitFiles() is in. I'll keep this open, as it's not actually solved, but remove it from v0.6.3.

@sjrd sjrd removed this from the v0.6.3 milestone May 1, 2015

@sjrd sjrd removed their assignment May 1, 2015

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly May 4, 2015

Contributor

@japgolly Would that be acceptable until they fix the issue in PhantomJS?

Should we provide PhantomJS2EnvWithConsoleErrorWorkaround defined above instead?

Anything that allows me to workaround this is acceptable to me, but that's my selfish view :) It might be better to have something provided because anyone who uses PhantomJS 2 is going to have this problem, and it could be a very long time before it's fixed upstream.

Contributor

japgolly commented May 4, 2015

@japgolly Would that be acceptable until they fix the issue in PhantomJS?

Should we provide PhantomJS2EnvWithConsoleErrorWorkaround defined above instead?

Anything that allows me to workaround this is acceptable to me, but that's my selfish view :) It might be better to have something provided because anyone who uses PhantomJS 2 is going to have this problem, and it could be a very long time before it's fixed upstream.

japgolly added a commit to japgolly/scalajs-react that referenced this issue May 28, 2015

Enable Travis CI again
Upgrading to Scala.JS 0.6.3 to workaround
scala-js/scala-js#1555
seems to fix it.
@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly Jun 6, 2015

Contributor

Using the PhantomJS2Env @gzm0 provided above solves this 98% of the time. (example).

Even with this fix applied however, this issue still occurs in rare occasions (like when there is an error initialising an object in a test). #1702 will address this.

If we apply #1702 and make PhantomJS2Env part of scala-js proper (preferably replacing PhantomJSEnv as it affects various versions of PhantomJS 1.x too), then this issue could (hopefully) be closed for good and for all users :)

Contributor

japgolly commented Jun 6, 2015

Using the PhantomJS2Env @gzm0 provided above solves this 98% of the time. (example).

Even with this fix applied however, this issue still occurs in rare occasions (like when there is an error initialising an object in a test). #1702 will address this.

If we apply #1702 and make PhantomJS2Env part of scala-js proper (preferably replacing PhantomJSEnv as it affects various versions of PhantomJS 1.x too), then this issue could (hopefully) be closed for good and for all users :)

gshakhn added a commit to gshakhn/private-wiki that referenced this issue Apr 16, 2016

Run tests in Firefox instead of PhantomJS.
Was hitting some issues (with uncommitted code) with PhantomJS.
zenoamaro/react-quill#87 was making React run console.error, which blows
up PhantomJS.

scala-js/scala-js#1555
ariya/phantomjs#13112

Running tests in Firefox seems more stable, though slower.
Unfortunately, I can't run headless firefox on Mac, which is annoying.
There is no X11 version of firefox available via brew
(Homebrew/legacy-homebrew#9150), or anywhere else apparently
(https://trac.macports.org/ticket/42087)

@gzm0 gzm0 added the wontfix label Nov 21, 2016

@gzm0

This comment has been minimized.

Show comment
Hide comment
@gzm0

gzm0 Nov 21, 2016

Contributor

Closing this as wontfix, since we'll be dropping PhantomJS.

Contributor

gzm0 commented Nov 21, 2016

Closing this as wontfix, since we'll be dropping PhantomJS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment