- Apr 22, 2019
-
-
Daniel Martí authored
Found by unparam.
-
Daniel Martí authored
If a page loads a child frame before the top frame has finished loading, we'd treat the child frame as the top-level frame, and the original frame would never get all of its nodes. This can be seen in the addition to TestLoadIframe; before the fix, that test would hang. The problem was that we treated all navigated frames as top-level frames, when that's not true. Only when a newly navigated frame has no parent frame should we treat it as our new top-level frame. Fixes #304.
-
Daniel Martí authored
While at it, waitQueue doesn't need to be passed the top-level frame.
-
Daniel Martí authored
For now, to at least have Travis be reliable.
-
- Apr 21, 2019
-
-
Daniel Martí authored
This time, the theory is that Travis is lucky enough to have the page load before the browser is finally killed. To get rid of this possibility, make the server not respond until the test finishes. This also simplifies the code a bit.
-
Daniel Martí authored
ExecAllocator did this already, but not RemoteAllocator. It was a bit tricky, as its Allocate happens with a completely separate context, so we can delay closing the websocket on cancel until the pages have been closed. To work around the issue, signal that the browser connection has been lost via a channel field on Browser, and not via Context. This works even when the context used in Allocate doesn't contain anything related to chromedp. This also means that Allocator implementations can have the same feature from third party packages, as the LostConnection field is exported.
-
Daniel Martí authored
To do this, we need to use a separate context for the websocket and browser, so that we can send the appropriate commands like Page.close. The code in RemoteAllocator is a bit hard-coded with the chromedp internals right now, but that can be improved later.
-
Daniel Martí authored
It doesn't close pages correctly yet (see the TODO), but at least the basics work.
-
Kenneth Shaw authored
Changes made to the Transport and Conn types to accommodate better testing of alternate websocket implementations.
-
- Apr 20, 2019
-
-
Kenneth Shaw authored
The CHROMEDP_DEBUG option introduced in last commit causes travis to cancel the build, since the log output is too long. This removes the environment variable.
-
Kenneth Shaw authored
In TestMain, adds recognition of CHROMEDP_DEBUG environment variable that when non-empty and not set to "false", adding `WithDebugf(log.Printf)` option to the Browser context for tests.
-
Daniel Martí authored
By reusing the lexer used to decode the nested object. While at it, deduplicate the custom easyjson.Unmarshal implementation code. name old time/op new time/op delta TabNavigate-8 27.4ms ± 5% 27.3ms ± 6% ~ (p=0.912 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 344kB ± 0% 344kB ± 0% ~ (p=0.165 n=10+10) name old allocs/op new allocs/op delta TabNavigate-8 990 ± 1% 952 ± 1% -3.89% (p=0.000 n=10+10)
-
Daniel Martí authored
name old time/op new time/op delta TabNavigate-8 27.3ms ± 3% 27.4ms ± 5% ~ (p=0.796 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 349kB ± 0% 344kB ± 0% -1.34% (p=0.000 n=9+10) name old allocs/op new allocs/op delta TabNavigate-8 1.03k ± 0% 0.99k ± 1% -3.83% (p=0.000 n=8+10)
-
Daniel Martí authored
Instead of one per Target, which adds a non-trivial amount of overhead once we have many targets on a single browser. The reduced overhead also means we can tick more often and gain a bit of performance. This results in a slight increase in allocs, since some wait functions allocate, but what matters is the elapsed time. name old time/op new time/op delta TabNavigate-8 28.0ms ± 4% 27.3ms ± 3% -2.44% (p=0.043 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 346kB ± 0% 349kB ± 0% +1.05% (p=0.000 n=10+9) name old allocs/op new allocs/op delta TabNavigate-8 985 ± 0% 1030 ± 0% +4.49% (p=0.000 n=10+8)
-
Daniel Martí authored
Before this, it was possible to create and cancel targets on a browser, and the map[target.SessionID]*Target would keep growing without ever shrinking back down. This wasn't caught by any of the tests or benchmarks though, as the map was initialized with a size of 1024 and the benchmark was usually run with N=150. In any case, this implementation is more correct, doesn't leak memory, and will scale better. No noticeable benchmark changes.
-
Daniel Martí authored
A single channel is enough. It's also better to set up Target and start its goroutine from newExecutorForTarget, as we don't take up resources on the goroutine that handles routing target events. name old time/op new time/op delta TabNavigate-8 29.3ms ± 5% 28.4ms ± 4% -2.90% (p=0.043 n=10+10)
-
Daniel Martí authored
It would sometimes fail with the "did not expect a nil error" failure. Intuitively, this could happen if Chrome takes too long to respond to os.Interrupt, and actually succeeds at loading the page. To make that impossible, send it os.Kill, which should exit the process immediately. If the flakes still happen, we can consider adding an extra wait on the process, or some form of sleep. Another bonus is that os.Kill works on Windows, so the test is now portable.
-
Daniel Martí authored
The easyjson Buffer type doesn't allow being dumped twice. That is, the BuildBytes call empties the buffer before returning, so the DumpTo call later wasn't writing anything. If we need to print to dbgf, use those bytes from BuildBytes directly to send to the websocket.
-
Daniel Martí authored
We can refactor cmdJob a bit to remove the need for two channel sends. They aren't trivially cheap, and the old logic was a bit overly complex. While at it, initialise cmdQueue as a buffered channel, to avoid unnecessary locking in Target.Execute. Do the same for an errc which is often used for only one error. name old time/op new time/op delta TabNavigate-8 28.5ms ± 6% 28.8ms ± 6% ~ (p=0.552 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 347kB ± 0% 346kB ± 0% -0.42% (p=0.001 n=9+10) name old allocs/op new allocs/op delta TabNavigate-8 1.00k ± 1% 0.98k ± 1% -1.78% (p=0.000 n=9+10)
-
Daniel Martí authored
For example, EventTargetReceivedMessageFromTarget is a message wrapping another message. The wrapping message is thrown away, so there's no need to allocate a new one every time we receive that event. That kind of event is very common, so this rewrite saves a noticeable amount of allocs. name old time/op new time/op delta TabNavigate-8 28.4ms ± 8% 28.5ms ± 6% ~ (p=0.739 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 350kB ± 0% 347kB ± 0% -0.88% (p=0.000 n=10+9) name old allocs/op new allocs/op delta TabNavigate-8 1.03k ± 0% 1.00k ± 1% -3.69% (p=0.000 n=10+9)
-
Daniel Martí authored
Removes an allocation per written message. Like in the Read case, this also lets us deduplicate the dbgf code. name old time/op new time/op delta TabNavigate-8 29.0ms ± 5% 28.4ms ± 8% ~ (p=0.190 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 352kB ± 0% 350kB ± 0% -0.69% (p=0.000 n=10+10) name old allocs/op new allocs/op delta TabNavigate-8 1.06k ± 1% 1.03k ± 0% -2.03% (p=0.000 n=10+10)
-
Daniel Martí authored
This function is called very often, so reducing one allocation per call is very significant and doesn't cost much code complexity. name old time/op new time/op delta TabNavigate-8 28.8ms ± 9% 29.0ms ± 5% ~ (p=0.579 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 359kB ± 0% 352kB ± 0% -1.96% (p=0.000 n=9+10) name old allocs/op new allocs/op delta TabNavigate-8 1.11k ± 1% 1.06k ± 1% -4.95% (p=0.000 n=10+10)
-
Daniel Martí authored
It uses ioutil.ReadAll, which doesn't reuse space. Rolling our own results in a pretty massive reduction in allocations. This also means we can deduplicate code with Conn.dbgf. name old time/op new time/op delta TabNavigate-8 29.3ms ± 6% 28.8ms ± 9% ~ (p=0.280 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 486kB ± 1% 359kB ± 0% -26.11% (p=0.000 n=10+9) name old allocs/op new allocs/op delta TabNavigate-8 1.21k ± 2% 1.11k ± 1% -8.26% (p=0.000 n=10+10)
-
Daniel Martí authored
We can't use cmd in the cancellation goroutine before setting up cmd; that's racey. 'go test -race' rightfully pointed this out: Write at 0x00c000010080 by goroutine 33: github.com/chromedp/chromedp.(*ExecAllocator).Allocate() /home/mvdan/src/chromedp/allocate.go:141 +0xb1a github.com/chromedp/chromedp.Run() /home/mvdan/src/chromedp/chromedp.go:178 +0x2a8 github.com/chromedp/chromedp.TestPrematureCancel() /home/mvdan/src/chromedp/chromedp_test.go:208 +0xc3 testing.tRunner() /home/mvdan/tip/src/testing/testing.go:865 +0x162 Previous read at 0x00c000010080 by goroutine 93: github.com/chromedp/chromedp.(*ExecAllocator).Allocate.func1() /home/mvdan/src/chromedp/allocate.go:120 +0x67
-
Daniel Martí authored
We receive a lot of events which are encapsulated for each target. Unfortunately, the actual event message is received as a string, which we then convert to []byte and unmarshal again. This means many allocations to unmarshal each of these event messages. A better approach is to perform a single unmarshal, decoding the underlying message directly from the input bytes. As expected, this results in a significant reduction in allocations. There's a tiny slowdown in comparison, but nothing significant given the high variance. encoding/json has the ",string" option precisely for this purpose. Unfortunately, easyjson doesn't have it, so roll our own with a custom type. In the future, I'd like to see this type in cdproto, but for now it can live here. While at it, slightly simplify some types. ok github.com/chromedp/chromedp 47.134s name old time/op new time/op delta TabNavigate-8 28.5ms ± 6% 29.3ms ± 6% ~ (p=0.218 n=10+10) name old alloc/op new alloc/op delta TabNavigate-8 498kB ± 1% 486kB ± 1% -2.30% (p=0.000 n=10+10) name old allocs/op new allocs/op delta TabNavigate-8 1.32k ± 1% 1.21k ± 2% -7.91% (p=0.000 n=10+10)
-
Daniel Martí authored
The old codebase ran them approximately every 5ms via sleeps. This had two major problems. First, we blocked the Target.run goroutine, meaning that we wouldn't be able to receive more events until after those 5ms. Second, the sleep would be a waste of time in some scenarios. For example, waiting for a node to be ready is a common occurrence, but it can very often be finished right after the next received DOM event. To fix the first issue, use a ticker, which doesn't stop our goroutine from doing other work like handling events. To fix the second, check the wait funcs after every DOM event. This does mean that we run the funcs more often, which allocate some objects, but this is still a net win in wall time. To help reduce the allocations a bit, remove the need for waitReady to allocate a []error and a WaitGroup every time it is run. name old time/op new time/op delta TabNavigate-8 31.3ms ± 2% 28.5ms ± 6% -8.72% (p=0.000 n=8+10) name old alloc/op new alloc/op delta TabNavigate-8 451kB ± 0% 498kB ± 1% +10.32% (p=0.000 n=9+10) name old allocs/op new allocs/op delta TabNavigate-8 1.03k ± 1% 1.32k ± 1% +27.91% (p=0.000 n=10+10)
-
Daniel Martí authored
Doesn't particularly give us a CPU win, but it does noticeably reduce allocations. name old time/op new time/op delta TabNavigate-8 30.9ms ± 3% 31.3ms ± 2% ~ (p=0.083 n=10+8) name old alloc/op new alloc/op delta TabNavigate-8 458kB ± 0% 451kB ± 0% -1.42% (p=0.000 n=10+9) name old allocs/op new allocs/op delta TabNavigate-8 1.19k ± 0% 1.03k ± 1% -13.49% (p=0.000 n=10+10)
-
Daniel Martí authored
This parallel benchmark simply opens a new tab on a browser, navigates to a local page, and waits for an element to be visible. Will be useful to monitor chromedp's overhead, for example when it comes to marshaling and unmarshaling json objects when talking to Chrome. name time/op TabNavigate-8 30.9ms ± 3% name alloc/op TabNavigate-8 458kB ± 0% name allocs/op TabNavigate-8 1.19k ± 0%
-
- Apr 18, 2019
-
-
Daniel Martí authored
The first would fail as we'd decrement the WaitGroup before adding to it, and the escond would fail with a nil pointer dereference panic. Add tests to reproduce both, and fix them.
-
- Apr 17, 2019
-
-
Daniel Martí authored
It's right that we were leaking a context here, resulting in a timer not being stopped once the test finished.
-
Daniel Martí authored
We have been developing this behavior over the past few weeks, but it wasn't properly documented anywhere. Fixes #303.
-
Daniel Martí authored
For example, this can be useful if a user wants to simply add one flag of their own, without otherwise messing with the default flags. In the old codebase, they'd either have to build their own list from scratch, or copy ours from source, needing to keep it in sync.
-
- Apr 16, 2019
-
-
Daniel Martí authored
We made the function error with ErrInvalidContext in that case, but the godoc was still a bit ambiguous. Make it clearer to avoid confusion. For #299.
-
- Apr 14, 2019
-
-
Daniel Martí authored
The correct way to use Run is after NewContext. Using NewExecAllocator only, which also gives a context, almost worked. This confused some users into thinking it was a bug in chromedp. Instead, error immediately with an invalid context error. Fixes #299.
-
- Apr 09, 2019
-
-
Daniel Martí authored
-
Daniel Martí authored
That way, cancelling a context while checking the error is much simpler. The context data already holds onto the cancel func, so this requires no internal changes. This renders the Browser.Shutdown API obsolete, even though it doesn't do exactly the same. If we want Cancel to do a proper shutdown action before cancelling a browser context and killing the process, we could do that change within the cancel logic.
-
Kenneth Shaw authored
Adds the high level WithDebugf() context option, and associated lower level browser and dial options for setting a protocol wire debugger. Additionally changes the conn.Conn.Read/Write implementations to be more efficient, using direct easyjson.{Marshal,Unmarshal} calls and logging to debug func when available.
-
- Apr 08, 2019
-
-
Daniel Martí authored
The API isn't very shiny, but it works. It doesn't matter that much, as most users won't care about these errors. Fixes #295.
-
Daniel Martí authored
While at it, remove the error return from BrowserOption, to make it consistent with all the other option func types. We don't have a good test for this feature yet, but at least we check that it doesn't crash or error via an example. Fixes #292.
-
Daniel Martí authored
For some reason, this test fails about half the time on Travis, but I can't get it to fail even after stress-testing hundreds of concurrent runs. It might be because Travis is on a much older version of Chrome. We'll fix that soon, by having chromedp select a specific version of Chrome. For now, make it more conservative, not assuming that a Location after a Navigate isn't racy.
-