Issue 6986 - Don't use chromium remote interface on Windows

[Sebastian Noack, 2018-09-25 22:12:47]

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
File test/runners/chromium_remote_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700);
fs.constants.S_IRWXU is 0o700. This change has no effect as far as I can tell.

[hub, 2018-09-26 01:03:25]

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
File test/runners/chromium_remote_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700);
On 2018/09/25 22:12:46, Sebastian Noack wrote:
> fs.constants.S_IRWXU is 0o700. This change has no effect as far as I can tell.

on Windows? I expect it to be undefined there.

[Sebastian Noack, 2018-09-26 01:10:51]

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
File test/runners/chromium_remote_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700);
On 2018/09/26 01:03:25, hub wrote:
> On 2018/09/25 22:12:46, Sebastian Noack wrote:
> > fs.constants.S_IRWXU is 0o700. This change has no effect as far as I can
tell.
> 
> on Windows? I expect it to be undefined there.

Geo, can you confirm?

[geo, 2018-09-26 13:34:09]

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
File test/runners/chromium_remote_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29891681/test/runners/chromi...
test/runners/chromium_remote_process.js:62: fs.chmodSync(chromiumPath, 0o700);
On 2018/09/26 01:10:51, Sebastian Noack wrote:
> On 2018/09/26 01:03:25, hub wrote:
> > On 2018/09/25 22:12:46, Sebastian Noack wrote:
> > > fs.constants.S_IRWXU is 0o700. This change has no effect as far as I can
> tell.
> > 
> > on Windows? I expect it to be undefined there.
> 
> Geo, can you confirm?

Well, it get's rid of the `fs.chmodSync` error. However now I'm having a
different error.

`
Error: The geckodriver.exe executable could not be found on the current PATH.
Please download the latest version from
https://github.com/mozilla/geckodriver/releases/ and ensure it can be found on
your PATH.
    at findGeckoDriver
(C:\Work\new\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\firefox\index.js:354:11)
    at new ServiceBuilder
(C:\Work\new\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\firefox\index.js:446:22)
    at Function.createSession
(C:\Work\new\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\firefox\index.js:516:21)
    at createDriver
(C:\Work\new\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\index.js:170:33)
    at Builder.build
(C:\Work\new\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\index.js:645:16)
    at runScript
(C:\Work\new\adblockpluschrome\adblockpluscore\test\runners\firefox_process.js:42:10)
    at ensureFirefox.then.firefoxPath
(C:\Work\new\adblockpluschrome\adblockpluscore\test\runners\firefox_process.js:51:12)
    at <anonymous>
`

On different clones of the repo, I'm not getting the error that I've reported,
but instead:

`
C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\lib\promise.js:2626
        throw error;
        ^

SessionNotCreatedError: Failed to start browser
C:\Work\adblockpluschrome\adblockpluscore\firefox-snapshots\firefox-win32-x64-57.0\firefox.exe:
no such file or directory
    at Object.throwDecodedError
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\lib\error.js:514:15)
    at parseHttpResponse
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\lib\http.js:519:13)
    at doSend.then.response
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\lib\http.js:441:30)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
From: Task: WebDriver.createSession()
    at Function.createSession
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\lib\webdriver.js:769:24)
    at Function.createSession
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\firefox\index.js:521:41)
    at createDriver
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\index.js:170:33)
    at Builder.build
(C:\Work\adblockpluschrome\adblockpluscore\node_modules\selenium-webdriver\index.js:645:16)
    at runScript
(C:\Work\adblockpluschrome\adblockpluscore\test\runners\firefox_process.js:42:10)
    at ensureFirefox.then.firefoxPath
(C:\Work\adblockpluschrome\adblockpluscore\test\runners\firefox_process.js:51:12)
    at <anonymous>
`

Kinda seems the same issue.

[geo, 2018-09-26 22:00:09]

I've tested the patch in combination with this
fix:https://hg.adblockplus.org/adblockpluscore/rev/50ac4b73ce21

Now I'm getting the following error:
`
 Error: Command failed:
C:\Work\23\adblockpluschrome\adblockpluscore\chromium-snapshots\chromium-win32-x64-467222\chrome-win32\chrome.exe
--headless --single-process --disable-gpu --no-sandbox
--allow-file-access-from-files --remote-debugging-port=9222
--user-data-dir=C:\Users\LOCALA~1\AppData\Local\Temp\chromium-datav3pN40
[12680:8064:0927/005810.800:FATAL:content_main_runner.cc(388)] --single-process
is not supported in chrome multiple dll browser.
Backtrace:
        GetHandleVerifier [0x00007FFF8A8B8E95+488293]
        GetHandleVerifier [0x00007FFF8A8BBD93+500323]
        RelaunchChromeBrowserWithNewCommandLineIfNeeded
[0x00007FFF8A835AEB+522511]
        ovly_debug_event [0x00007FFF8A75C7E4+5038292]
        ovly_debug_event [0x00007FFF8A75C6F0+5038048]
        IsSandboxedProcess [0x00007FFF8B256956+14954]
        ovly_debug_event [0x00007FFF8A75BDFC+5035756]
        ChromeMain [0x00007FFF89F2BD60+240]
        (No symbol) [0x00007FF6E5C45ED0]
        (No symbol) [0x00007FF6E5C45524]
        IsSandboxedProcess [0x00007FF6E5CA6603+130307]
        BaseThreadInitThunk [0x00007FFFDF333034+20]
        RtlUserThreadStart [0x00007FFFE19F1461+33]

[0927/005810.831:ERROR:process_info.cc(631)] range at 0x515452bc00000000, size
0x1d8 fully unreadable
[0927/005810.831:ERROR:process_info.cc(631)] range at 0x515452dc00000000, size
0x1d8 fully unreadable
[0927/005810.831:ERROR:process_info.cc(631)] range at 0x515452de00000000, size
0x1d8 fully unreadable

    at ChildProcess.exithandler (child_process.js:276:12)
    at emitTwo (events.js:126:13)
    at ChildProcess.emit (events.js:214:7)
    at maybeClose (internal/child_process.js:915:16)
    at Socket.stream.socket.on (internal/child_process.js:336:11)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at Pipe._handle.close [as _onclose] (net.js:561:12)
  killed: false,
  code: 2147483651,
  signal: null,
  cmd:
'C:\\Work\\23\\adblockpluschrome\\adblockpluscore\\chromium-snapshots\\chromium-win32-x64-467222\\chrome-win32\\chrome.exe
--headless --single-process --disable-gpu --no-sandbox
--allow-file-access-from-files --remote-debugging-port=9222
--user-data-dir=C:\\Users\\LOCALA~1\\AppData\\Local\\Temp\\chromium-datav3pN40'
`

[hub, 2018-09-27 12:47:05]

On 2018/09/26 22:00:09, geo wrote:
> I've tested the patch in combination with this
> fix:https://hg.adblockplus.org/adblockpluscore/rev/50ac4b73ce21
> 
> Now I'm getting the following error:
> `
>  Error: Command failed:
>
C:\Work\23\adblockpluschrome\adblockpluscore\chromium-snapshots\chromium-win32-x64-467222\chrome-win32\chrome.exe
> --headless --single-process --disable-gpu --no-sandbox
> --allow-file-access-from-files --remote-debugging-port=9222
> --user-data-dir=C:\Users\LOCALA~1\AppData\Local\Temp\chromium-datav3pN40
> [12680:8064:0927/005810.800:FATAL:content_main_runner.cc(388)]
--single-process
> is not supported in chrome multiple dll browser.

I don't know what a "chrome multiple dll browser" is but it seems that
--single-process is passed by default by selenium-webdriver, and it doesn't seem
to be in the Javascript module.

[Sebastian Noack, 2018-09-27 15:53:25]

FWIW, in adblockpluschrome the browser tests pass on Windows now. We use the
exact same version of Chromium, selenium-webdriver and chromedriver there, and
the only apparent difference left in the code initializing the driver I see is
that we don't call .headless().

More information on the multiple DLL architecture:
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/windows_split_dll.md

[hub, 2018-09-27 16:52:39]

On 2018/09/27 15:53:25, Sebastian Noack wrote:
> FWIW, in adblockpluschrome the browser tests pass on Windows now. We use the
> exact same version of Chromium, selenium-webdriver and chromedriver there, and
> the only apparent difference left in the code initializing the driver I see is
> that we don't call .headless().

Let's disable headless on Windows (for adblockpluscore)

[hub, 2018-09-27 17:01:02]

I have updated the patch to disable headless on Chrome on Windows.

[Sebastian Noack, 2018-09-28 15:37:00]

Geo, can you try if the tests pass on Windows after applying the latest patch
set?

https://codereview.adblockplus.org/29891680/diff/29893612/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29893612/test/runners/chromi...
test/runners/chromium_process.js:36: let {platform} = process;
Nit: This doesn't seem to be worth a temporary variable, you can just inline
process.platform below.

[hub, 2018-09-28 15:56:35]

Updated patch

https://codereview.adblockplus.org/29891680/diff/29893612/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29893612/test/runners/chromi...
test/runners/chromium_process.js:36: let {platform} = process;
On 2018/09/28 15:37:00, Sebastian Noack wrote:
> Nit: This doesn't seem to be worth a temporary variable, you can just inline
> process.platform below.

Done.

[geo, 2018-10-01 13:19:04]

https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromi...
File test/runners/chromium_remote_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromi...
test/runners/chromium_remote_process.js:72: "--headless", "--single-process",
"--disable-gpu", "--no-sandbox",
I've tested the latest patch, but I'm still getting that `--single-process is
not supported in chrome multiple dll browser` error.

I removed the "--single-process" argument (just to see what happens) and now I
have a different error,

`
Browser unit test failed: EvalError: Refused to evaluate a string as JavaScript
because 'unsafe-eval' is not an allowed source of script in the following
Content Security Policy directive: "script-src 'self' blob: filesystem:
chrome-extension-resource:".


    at Object.<anonymous> (bundle.js:71:1)
    at __webpack_require__ (bundle.js:20:30)
    at bundle.js:63:18
    at bundle.js:66:10
`
I'm not sure if that was a step in the right direction or not, as I'm really new
to this whole thing, however if you need me to test anything else (even before
uploading a new patch), let me know, I'm more than happy to assist.

[hub, 2018-10-30 23:39:45]

Updated patch

https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromi...
File test/runners/chromium_remote_process.js (right):

https://codereview.adblockplus.org/29891680/diff/29894590/test/runners/chromi...
test/runners/chromium_remote_process.js:72: "--headless", "--single-process",
"--disable-gpu", "--no-sandbox",
On 2018/10/01 13:19:03, geo wrote:
> I've tested the latest patch, but I'm still getting that `--single-process is
> not supported in chrome multiple dll browser` error.
> 
> I removed the "--single-process" argument (just to see what happens) and now I
> have a different error,
> 
> `
> Browser unit test failed: EvalError: Refused to evaluate a string as
JavaScript
> because 'unsafe-eval' is not an allowed source of script in the following
> Content Security Policy directive: "script-src 'self' blob: filesystem:
> chrome-extension-resource:".
> 
> 
>     at Object.<anonymous> (bundle.js:71:1)
>     at __webpack_require__ (bundle.js:20:30)
>     at bundle.js:63:18
>     at bundle.js:66:10
> `
> I'm not sure if that was a step in the right direction or not, as I'm really
new
> to this whole thing, however if you need me to test anything else (even before
> uploading a new patch), let me know, I'm more than happy to assist.

I removed the command line option, on win32, this time. But this is weird that
you get the error on Windows but we don't elsewhere....

[hub, 2018-11-08 21:25:40]

updated patch

Now we default to webdriver Chromium on windows.

[geo, 2018-11-08 21:32:01]

On 2018/11/08 21:25:40, hub wrote:
> updated patch
> 
> Now we default to webdriver Chromium on windows.

Now it works on Windows, so LGTM.

[hub, 2018-11-09 18:20:38]

Added Tristan as a reviewer since it is "Automation"

Manish in CC

[Sebastian Noack, 2018-11-09 19:07:46]

Are the changes in chromium_remote_process.js necessary if we only use the web
driver implementation on Windows?

[hub, 2018-11-09 19:50:43]

On 2018/11/09 19:07:46, Sebastian Noack wrote:
> Are the changes in chromium_remote_process.js necessary if we only use the web
> driver implementation on Windows?

I think it should work without.

[hub, 2018-11-09 20:03:10]

updated patch

[Sebastian Noack, 2018-11-09 20:17:27]

LGTM