also looking at this
fix(android): tie a ready mark to its own navigation, and inject a document once
#239
5 files
+159
-3
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -348,7 +348,7 @@ describe('Craft Android builder', () => { | ||
| 348 | 348 | expect(activity).toContain('craftBridge.restoreDeepLinks(savedInstanceState)') |
| 349 | 349 | |
| 350 | 350 | // Flushed in the injection's completion, after the page's replay exists. |
| 351 | const injected = bridge.slice(bridge.indexOf('webView.evaluateJavascript(script) {')) | |
| 351 | const injected = bridge.slice(bridge.indexOf('webView.evaluateJavascript(onlyOnce(script)) {')) | |
| 352 | 352 | const flush = injected.slice(0, injected.indexOf('fun markBridgeLoading()')) |
| 353 | 353 | expect(flush.indexOf('bridgeReady = true')).toBeLessThan(flush.indexOf('links.forEach { (url, initial) -> dispatchDeepLink(url, initial) }')) |
| 354 | 354 | |
| @@ -413,6 +413,31 @@ describe('Craft Android builder', () => { | ||
| 413 | 413 | expect(bridge).toContain('sendEvent("craftSpeechEnd", emptyMap())') |
| 414 | 414 | }) |
| 415 | 415 | |
| 416 | it('marks ready only the page its injection ran in, and injects a document once', async () => { | |
| 417 | // #228, two halves. The evaluateJavascript completion is posted, so it | |
| 418 | // can land after the next navigation has started and mark a page that | |
| 419 | // was never injected as ready — flushing the deep-link and shortcut | |
| 420 | // queues into a document with no bridge. And onPageFinished arrives | |
| 421 | // without onPageStarted for a same-document navigation, so the script | |
| 422 | // could run twice in one document. | |
| 423 | const output = mkdtempSync(join(tmpdir(), 'craft-android-injection-')) | |
| 424 | await init({ name: 'WildLoop', packageName: 'org.wildloop.app', output }) | |
| 425 | ||
| 426 | const bridge = readFileSync(join(output, 'app/src/main/java/org/wildloop/app/CraftBridge.kt'), 'utf8') | |
| 427 | expect(bridge).toContain('private var navigationGeneration = 0') | |
| 428 | expect(bridge).toContain('navigationGeneration += 1') | |
| 429 | expect(bridge).toContain('val generation = navigationGeneration') | |
| 430 | expect(bridge).toContain('if (generation != navigationGeneration) return@evaluateJavascript') | |
| 431 | // The generation is taken before the script is handed over, and compared | |
| 432 | // after; either order alone leaves the race open. | |
| 433 | expect(bridge.indexOf('val generation = navigationGeneration')) | |
| 434 | .toBeLessThan(bridge.indexOf('if (generation != navigationGeneration)')) | |
| 435 | ||
| 436 | expect(bridge).toContain('private fun onlyOnce(script: String): String') | |
| 437 | expect(bridge).toContain('evaluateJavascript(onlyOnce(script))') | |
| 438 | expect(bridge).toContain('if (!window.__craftBridgeInstalled)') | |
| 439 | }) | |
| 440 | ||
| 416 | 441 | it('routes external Activity results back to every pending media promise', async () => { |
| 417 | 442 | const output = mkdtempSync(join(tmpdir(), 'craft-android-activity-results-')) |
| 418 | 443 | await init({ name: 'WildLoop', packageName: 'org.wildloop.app', output }) |
| @@ -30,6 +30,18 @@ function pageScript(enabled = true): string { | ||
| 30 | 30 | return script |
| 31 | 31 | } |
| 32 | 32 | |
| 33 | /** | |
| 34 | * The script as Kotlin actually evaluates it: wrapped by `onlyOnce`, which is | |
| 35 | * what keeps a second injection into the same document from tearing the | |
| 36 | * bridge down and building it again (#228). | |
| 37 | */ | |
| 38 | function injectedScript(enabled = true): string { | |
| 39 | const wrapper = template.match(/private fun onlyOnce\(script: String\): String =\n\s*"([^"]*)\$script([^"]*)"/) | |
| 40 | if (!wrapper) throw new Error('CraftBridge.kt.template no longer guards the script with onlyOnce') | |
| 41 | const unescape = (part: string) => part.replace(/\\n/g, '\n') | |
| 42 | return `${unescape(wrapper[1]!)}${pageScript(enabled)}${unescape(wrapper[2]!)}` | |
| 43 | } | |
| 44 | ||
| 33 | 45 | type Listener = (event: { type: string, detail: unknown }) => void |
| 34 | 46 | |
| 35 | 47 | /** |
| @@ -84,7 +96,7 @@ function loadPage( | ||
| 84 | 96 | |
| 85 | 97 | const inject = () => { |
| 86 | 98 | // eslint-disable-next-line no-new-func |
| 87 | new Function('window', 'document', 'CraftAndroid', 'CustomEvent', 'console', pageScript(enabled))( | |
| 99 | new Function('window', 'document', 'CraftAndroid', 'CustomEvent', 'console', injectedScript(enabled))( | |
| 88 | 100 | page, |
| 89 | 101 | document, |
| 90 | 102 | CraftAndroid, |
| @@ -371,4 +383,40 @@ describe('the injected Android page script', () => { | ||
| 371 | 383 | expect(first).toEqual([{ url: 'app://warm', initial: false }]) |
| 372 | 384 | expect(second).toEqual([]) |
| 373 | 385 | }) |
| 386 | ||
| 387 | // #228: Chromium calls onPageFinished without onPageStarted for a | |
| 388 | // same-document navigation — a fragment change, a history.pushState — and | |
| 389 | // MainActivity injects on every one. Running the script again used to | |
| 390 | // replace window.craft, fire craftReady twice, and reject everything in | |
| 391 | // flight with "Android bridge reinitialized". | |
| 392 | it('leaves the bridge alone when the same document is injected again', async () => { | |
| 393 | const page = loadPage() | |
| 394 | const installed = page.craft | |
| 395 | let readyAgain = 0 | |
| 396 | page.page.addEventListener('craftReady', () => { readyAgain += 1 }) | |
| 397 | ||
| 398 | page.inject() | |
| 399 | ||
| 400 | expect(page.craft).toBe(installed) | |
| 401 | expect(readyAgain).toBe(0) | |
| 402 | }) | |
| 403 | ||
| 404 | it('does not reject an in-flight promise when the same document is injected again', async () => { | |
| 405 | const page = loadPage() | |
| 406 | const pending = page.craft.share('hello') | |
| 407 | page.inject() | |
| 408 | ||
| 409 | // Settle it the way Kotlin does. A second injection used to reject this | |
| 410 | // first, and the page would have seen a share it never cancelled fail. | |
| 411 | page.page._craftShareResolve(true) | |
| 412 | expect(await pending).toBe(true) | |
| 413 | }) | |
| 414 | ||
| 415 | it('installs into a document that has never had it', async () => { | |
| 416 | // The other half: a real navigation gets a new window, so the flag is | |
| 417 | // absent and the guard must not stop the bridge appearing at all. | |
| 418 | const page = loadPage() | |
| 419 | expect(typeof page.craft).toBe('object') | |
| 420 | expect(page.page.__craftBridgeInstalled).toBe(true) | |
| 421 | }) | |
| 374 | 422 | }) |
| @@ -219,6 +219,19 @@ class CraftBridge( | ||
| 219 | 219 | private val mainExecutor: Executor = ContextCompat.getMainExecutor(activity) |
| 220 | 220 | @Volatile |
| 221 | 221 | private var bridgeReady = false |
| 222 | ||
| 223 | /** | |
| 224 | * Which navigation the bridge belongs to. | |
| 225 | * | |
| 226 | * `evaluateJavascript`'s completion is posted, so it can run after the | |
| 227 | * next navigation has already started. Without this, a completion from | |
| 228 | * the page that has just left marks the page that has just arrived as | |
| 229 | * ready, and the queues below flush into a document with no bridge and no | |
| 230 | * replay listener — where they are lost rather than delivered (#228). | |
| 231 | * | |
| 232 | * Main thread only, like everything that touches `bridgeReady`. | |
| 233 | */ | |
| 234 | private var navigationGeneration = 0 | |
| 222 | 235 | private val pendingEvents = mutableListOf<Pair<String, Map<String, Any>>>() |
| 223 | 236 | |
| 224 | 237 | // What the app was built with (#209). The page refuses a disabled call |
| @@ -327,6 +340,24 @@ class CraftBridge( | ||
| 327 | 340 | */ |
| 328 | 341 | private fun jsQuote(value: Any?): String = JSONObject.quote(value.toString()) |
| 329 | 342 | |
| 343 | /** | |
| 344 | * The script, guarded so a second run in the *same* document does nothing. | |
| 345 | * | |
| 346 | * Chromium calls `onPageFinished` without a matching `onPageStarted` for | |
| 347 | * same-document navigations — a fragment change, a `history.pushState` — | |
| 348 | * and `MainActivity` injects on every one of them. Running the whole | |
| 349 | * script again replaces `window.craft`, fires `craftReady` a second time, | |
| 350 | * and rejects every in-flight promise with "Android bridge reinitialized" | |
| 351 | * (#228). | |
| 352 | * | |
| 353 | * A block, not a function: `var` and function declarations are | |
| 354 | * function-scoped, so the script behaves exactly as it does unwrapped, | |
| 355 | * and everything it publishes it publishes on `window` regardless. A real | |
| 356 | * navigation gets a new `window`, so the flag is absent and it installs. | |
| 357 | */ | |
| 358 | private fun onlyOnce(script: String): String = | |
| 359 | "if (!window.__craftBridgeInstalled) {\nwindow.__craftBridgeInstalled = true;\n$script\n}" | |
| 360 | ||
| 330 | 361 | fun injectBridge() { |
| 331 | 362 | // Before the script, so a native callback arriving while the page is |
| 332 | 363 | // still loading has somewhere to go rather than being counted as |
| @@ -1653,8 +1684,15 @@ class CraftBridge( | ||
| 1653 | 1684 | |
| 1654 | 1685 | activity.runOnUiThread { |
| 1655 | 1686 | if (closed) return@runOnUiThread |
| 1656 | webView.evaluateJavascript(script) { | |
| 1687 | // Read here rather than at entry: this is the navigation the | |
| 1688 | // script is actually being injected into. | |
| 1689 | val generation = navigationGeneration | |
| 1690 | webView.evaluateJavascript(onlyOnce(script)) { | |
| 1657 | 1691 | if (closed) return@evaluateJavascript |
| 1692 | // A completion for a page that has since navigated away. | |
| 1693 | // Marking ready now would flush into the new document, which | |
| 1694 | // has no bridge yet, and the queues would be spent on it. | |
| 1695 | if (generation != navigationGeneration) return@evaluateJavascript | |
| 1658 | 1696 | bridgeReady = true |
| 1659 | 1697 | val events = pendingEvents.toList() |
| 1660 | 1698 | pendingEvents.clear() |
| @@ -1670,6 +1708,9 @@ class CraftBridge( | ||
| 1670 | 1708 | |
| 1671 | 1709 | fun markBridgeLoading() { |
| 1672 | 1710 | bridgeReady = false |
| 1711 | // A document is on its way out, so any injection still in flight is | |
| 1712 | // for a page that no longer exists. | |
| 1713 | navigationGeneration += 1 | |
| 1673 | 1714 | } |
| 1674 | 1715 | |
| 1675 | 1716 | // ==================== Haptics ==================== |
| @@ -381,6 +381,47 @@ | ||
| 381 | 381 | }) |
| 382 | 382 | } |
| 383 | 383 | }, |
| 384 | { | |
| 385 | // #228: Chromium calls onPageFinished without a matching | |
| 386 | // onPageStarted for a same-document navigation, and MainActivity | |
| 387 | // injects the bridge on every one of them. Whether this emulator | |
| 388 | // does it is what the issue asked to find out; either way the page | |
| 389 | // must come out of it with the bridge it started with, because a | |
| 390 | // second run of the script replaces window.craft, fires craftReady | |
| 391 | // again, and rejects everything in flight. | |
| 392 | // | |
| 393 | // The wait is what makes this a real check: onPageFinished arrives | |
| 394 | // asynchronously, so a case that asserted immediately would pass | |
| 395 | // without having given the WebView a chance to re-inject. | |
| 396 | name: 'bridge.survivesSameDocumentNavigation', | |
| 397 | platforms: ['android'], | |
| 398 | run: function () { | |
| 399 | var installed = window.craft | |
| 400 | // The runtime that holds every pending promise. A second injection | |
| 401 | // replaces it, and on the way out rejects them all with "Android | |
| 402 | // bridge reinitialized" — which is the damage this case is about, | |
| 403 | // and is visible here without needing a call that stays pending | |
| 404 | // for the whole wait. | |
| 405 | var runtime = window.__craftPromise | |
| 406 | var readyAgain = 0 | |
| 407 | var onReady = function () { readyAgain += 1 } | |
| 408 | window.addEventListener('craftReady', onReady) | |
| 409 | ||
| 410 | history.pushState({}, '', location.pathname + '?craft-e2e-pushstate=1') | |
| 411 | ||
| 412 | return new Promise(function (resolve) { setTimeout(resolve, 2000) }).then(function () { | |
| 413 | window.removeEventListener('craftReady', onReady) | |
| 414 | if (window.craft !== installed) return 'window.craft was replaced by a second injection' | |
| 415 | if (window.__craftPromise !== runtime) return 'the promise runtime was replaced, rejecting everything in flight' | |
| 416 | if (readyAgain !== 0) return 'craftReady fired ' + readyAgain + ' more time(s) in the same document' | |
| 417 | // And the bridge still works afterwards, so the guard cannot pass | |
| 418 | // by having stopped the bridge from existing at all. | |
| 419 | return withTimeout(function () { return window.craft.clipboard.read() }).then(function (outcome) { | |
| 420 | if (outcome.state !== 'resolved') return 'the bridge stopped answering after the navigation' | |
| 421 | }) | |
| 422 | }) | |
| 423 | } | |
| 424 | }, | |
| 384 | 425 | { |
| 385 | 426 | // #209: a capability the app was built with is served, and the page |
| 386 | 427 | // can tell which is which from capabilities alone. Sharing is on in |