javascript modules not working with cache-api
Reported by
filipb...@filipbech.dk,
Apr 12 2018
|
||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36 Steps to reproduce the problem: 1. Add multiple (more than one) javascript files (With module syntax e.g. import/export etc) to a cache.addAll() from event.waitUntill in sw install-event. What is the expected behavior? The files should appear in the cache What went wrong? Browser crashes! (I reproduced on two different browsers, but on a third it worked fine) Also, If I only have óne file, a syntax-error is raised (that you can catch) - but the file is still put in the cache. But even if I could make it with just one file, the cache.match(event.request) doesn't match any request I can make myself with the cache.add-api... Did this work before? N/A Chrome version: 65.0.3325.181 Channel: stable OS Version: OS X 10.13.4 Flash Version: I made 4 different reproductions (in the zip-file and on github (source: https://github.com/filipbech/filipbech.github.io/tree/master/demos/module-bug/) 1. as described liveurl: https://filipbech.github.io/demos/module-bug/repro1/ BROWSER CRASHES 2. using type=module-like requests (@jeffposnick pointed me here) liveurl:https://filipbech.github.io/demos/module-bug/repro2/ BROWSER CRASHES 3. "faking" what cache.addAll does liveurl: https://filipbech.github.io/demos/module-bug/repro3/ Syntax-errors from module-syntax (even if the file is not referenced elsewhere - so should never be excecuted) 4. caching the files from the fetch-event liveurl: https://filipbech.github.io/demos/module-bug/repro4/ Works as intended, but not optimal for critical resources What I'm trying to do is just put a set of native javascript modules in the cache to make it offline-capable.
,
Apr 13 2018
,
Apr 13 2018
Repros for me in stable. Fun!
,
Apr 13 2018
Relevant parts of stack frame: #3 blink::NonThrowableExceptionState::RethrowV8Exception() #4 blink::ReadableStreamOperations::GetReader() #5 blink::BodyStreamBuffer::CloseAndLockAndDisturb() #6 blink::BodyStreamBuffer::ReleaseHandle() #7 blink::BodyStreamBuffer::StartLoading() #8 blink::Cache::PutImpl() #9 blink::Cache::FetchResolvedForAdd::Call()
,
Apr 13 2018
... which is in code specific to the V8 Code Cache. The invariant in BodyStreamBuffer that the eventual ReadableStreamOperations::GetReader() operation can't throw appears to be violated when ES Modules are in use?
,
Apr 16 2018
Issue 833501 has been merged into this issue.
,
Apr 17 2018
,
Apr 17 2018
This is a bug of PWAFullCodeCache. CodeCacheHandleCallbackForPut is generating V8 code cache of a classic script file. This is a bug that after CodeCacheHandleCallbackForPut failed to generate the code cache (this happens when the file is a ES module), the script state becomes strange state. I will disable this feature.
,
Apr 17 2018
When I add "v8::TryCatch block(isolate);" in V8ScriptRunner::GenerateFullCodeCache(), the bug looks fixed. https://chromium-review.googlesource.com/c/chromium/src/+/1014744 yangguo@ Is it needed to use v8::TryCatch before calling ScriptCompiler::CompileUnboundScript not to cause side effects on the script state?
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5 commit b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5 Author: Tsuyoshi Horo <horo@chromium.org> Date: Tue Apr 17 05:44:17 2018 Revert "Enable ServiceWorkerScriptFullCodeCache by default" This reverts commit c4334bc5b70e16494bad3511562a045f5ccb70a7. Reason for revert: Caused crashes when storing javascript modules in cache storage. https://crbug.com/832202 Original change's description: > Enable ServiceWorkerScriptFullCodeCache by default > > Bug: 768705 ,788619 > Change-Id: I18474261a0bb443b5ed7186adb8bc68227ece26c > Reviewed-on: https://chromium-review.googlesource.com/994492 > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Cr-Commit-Position: refs/heads/master@{#547946} TBR=horo@chromium.org,kinuko@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 768705 , 788619, 832202 Change-Id: I53d3a401dfb83fbf54184588ca889b1a4f9f0474 Reviewed-on: https://chromium-review.googlesource.com/1014310 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#551258} [modify] https://crrev.com/b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5/content/public/common/content_features.cc
,
Apr 17 2018
,
Apr 17 2018
I'm at BlinkOn this week and won't have time to investigate this in detail. Where exactly does the exception occur? When compiling the unbound script? How come a module is compiled as unbound script in the first place? If https://chromium-review.googlesource.com/c/chromium/src/+/1014744 indeed makes the crash go away, then my current (unconfirmed) theory is that: - Somehow we compile a module with v8::ScriptCompiler::CompileUnboundScript. - Which we should not, since we have v8::ScriptCompiler::CompileModule instead. - This somehow triggers an exception to be thrown. - But GenerateFullCodeCache is not prepared to catch exceptions, even though it checks the result of the compilation. - Inserting a TryCatch is the right thing. Otherwise the exception persists and will cause issues later on, which explains the stack trace in #4. I'm not entirely sure how switching to eager compilation would trigger this issue. Maybe CompileUnboundScript only throws for modules if compiling eagerly? v8::ScriptCompiler::CreateCodeCache also doesn't support modules btw. I've talked to adamk@ about this a long while ago but I don't think anything happened here. We would have to serialize the uninstantiated module.
,
Apr 17 2018
I think the solution for now is that we don't generate code cache for ES modules at all.
,
Apr 17 2018
But aside from that having a TryCatch scope in GenerateFullCodeCache is a good idea anyways. Do we have tests for when we run GenerateFullCodeCache on a script with syntax errors? That might already cause issues.
,
Apr 17 2018
Currently, PWAFullCodeCache dosen't support modules. See the design doc: https://goo.gl/Pffqo7 "Currently we don't have plans to generate the full code cache for ES6 modules in ServiceWorker. But ServiceWorker can’t know whether the file should be loaded as a module or as a normal script. So ServiceWorker tries to generate full code cache as a script even if the file is a module. " GenerateFullCodeCache() is throwing the uncaught excetion not only for modules but alos for invalid text (ex "(,);"). I created a LayoutTest https://chromium-review.googlesource.com/c/chromium/src/+/1014744/2.
,
Apr 17 2018
,
Apr 17 2018
,
Apr 17 2018
Using this "Service Worker ECMAScript Modules Issue Tester": https://swesmit.hwalab.com/ (with source code at: https://github.com/hwalab-developer/swesmit) you can easily see the errors generated by using JavaScript modules with Cache API in Service Workers. The errors are displayed both in console, and in the browser (in the document itself), as in the attached screenshot. The browser version (the navigator.userAgent) is also displayed. In Chrome 65.0.3325.181 on Windows 10 I'm getting errors in the "app.js" script type module, and in the "hello.js" ECMAScript module. All other files are cached just fine, including the "classic.js" JavaScript file: [SW] cache.add /scripts/app.js SyntaxError: Unexpected identifier [SW] cache.add /scripts/hello.js SyntaxError: Unexpected token export [SW] cache.add /styles/main.css [SW] cache.add /scripts/classic.js [SW] cache.add /styles/normalize.css [SW] cache.add index.html When using cache.addAll(), the browser crashes while trying to cache both "app.js" and "hello.js". Trying to cache only one of them gives an error, but doesn't crash the browser.
,
Apr 17 2018
The issue also appears in Chrome 65.0.3325.109 on Android 6.0.1, please see the attached screenshots. Steps to reproduce the problem: Open https://swesmit.hwalab.com in Chrome. (I have also reproduced the issue on Android 7.0)
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c46ccbae6b623bea947f4cd5c92c19961c20f372 commit c46ccbae6b623bea947f4cd5c92c19961c20f372 Author: Tsuyoshi Horo <horo@chromium.org> Date: Wed Apr 18 22:27:04 2018 Use v8::TryCatch in V8ScriptRunner::GenerateFullCodeCache v8::TryCatch is needed to suppress all exceptions thrown during the code cache generation. Bug: 832202 Change-Id: I024adcdda2457f8db667d742e6175b77d2abf8e8 Reviewed-on: https://chromium-review.googlesource.com/1014744 Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#551855} [add] https://crrev.com/c46ccbae6b623bea947f4cd5c92c19961c20f372/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.invalid-code-cache.html [add] https://crrev.com/c46ccbae6b623bea947f4cd5c92c19961c20f372/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/invalid-code-cache-worker.js [modify] https://crrev.com/c46ccbae6b623bea947f4cd5c92c19961c20f372/third_party/blink/renderer/bindings/core/v8/v8_script_runner.cc
,
Apr 19 2018
,
Apr 19 2018
,
Apr 19 2018
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c8633b7d44e30d1d48553eeb545884c45c9af21c commit c8633b7d44e30d1d48553eeb545884c45c9af21c Author: Tsuyoshi Horo <horo@chromium.org> Date: Thu Apr 19 00:31:31 2018 Reland "Enable ServiceWorkerScriptFullCodeCache by default" This reverts commit b2eb7350ea8a03c5d9dca7dc7fa8068100560bf5. Reason for revert: The CL (crrev.com/c/1014744) which fixes the crash bug landed. Original change's description: > Revert "Enable ServiceWorkerScriptFullCodeCache by default" > > This reverts commit c4334bc5b70e16494bad3511562a045f5ccb70a7. > > Reason for revert: Caused crashes when storing javascript modules in cache storage. https://crbug.com/832202 > > Original change's description: > > Enable ServiceWorkerScriptFullCodeCache by default > > > > Bug: 768705 ,788619 > > Change-Id: I18474261a0bb443b5ed7186adb8bc68227ece26c > > Reviewed-on: https://chromium-review.googlesource.com/994492 > > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#547946} > > TBR=horo@chromium.org,kinuko@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 768705 , 788619, 832202 > Change-Id: I53d3a401dfb83fbf54184588ca889b1a4f9f0474 > Reviewed-on: https://chromium-review.googlesource.com/1014310 > Reviewed-by: Tsuyoshi Horo <horo@chromium.org> > Commit-Queue: Tsuyoshi Horo <horo@chromium.org> > Cr-Commit-Position: refs/heads/master@{#551258} TBR=horo@chromium.org,kinuko@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 768705 , 788619, 832202 Change-Id: I052ec2e159627facff21dda99f3fbfe15326aa6a Reviewed-on: https://chromium-review.googlesource.com/1018044 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/heads/master@{#551897} [modify] https://crrev.com/c8633b7d44e30d1d48553eeb545884c45c9af21c/content/browser/service_worker/service_worker_browsertest.cc [modify] https://crrev.com/c8633b7d44e30d1d48553eeb545884c45c9af21c/content/public/common/content_features.cc
,
Apr 19 2018
Re #22, could you pls clarify why this merge is important for M67 as bug reported here exists since M65? Also c46ccbae6b623bea947f4cd5c92c19961c20f372 just landed in trunk so it didn't bake in canary yet.
,
Apr 19 2018
PWAFullCodeCache has a big performance impact on PWA sites. Due to this crash issue, we had to disable PWAFullCodeCache feature. We already disabled it in M65 using Finch. If we can merge the cl to M67, we can re-enable PWAFullCodeCache from M67. I will not merge to M67 until it will well baked in canary.
,
Apr 19 2018
Pls update the bug with canary result. Thank you.
,
Apr 19 2018
The NextAction date has arrived: 2018-04-19
,
Apr 19 2018
I verified that Chrome 68.0.3400.0 doesn't crash while opening https://filipbech.github.io/demos/module-bug/repro1/ even if chrome://flags/#enable-pwa-full-code-cache is enabled.
,
Apr 19 2018
https://storage.googleapis.com/chromium-find-releases-static/c46.html#c46ccbae6b623bea947f4cd5c92c19961c20f372 > Commit c46ccbae... initially landed in 68.0.3400.0
,
Apr 19 2018
Approving merge to M67 branch 3396 based on comments #26 and #29. Please merge ASAP. Thank you.
,
Apr 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7336b1875146492d1c7c41c4bcf63e7f851b5410 commit 7336b1875146492d1c7c41c4bcf63e7f851b5410 Author: Tsuyoshi Horo <horo@chromium.org> Date: Thu Apr 19 15:28:27 2018 Use v8::TryCatch in V8ScriptRunner::GenerateFullCodeCache v8::TryCatch is needed to suppress all exceptions thrown during the code cache generation. Bug: 832202 Change-Id: I024adcdda2457f8db667d742e6175b77d2abf8e8 Reviewed-on: https://chromium-review.googlesource.com/1014744 Commit-Queue: Tsuyoshi Horo <horo@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#551855}(cherry picked from commit c46ccbae6b623bea947f4cd5c92c19961c20f372) Reviewed-on: https://chromium-review.googlesource.com/1019680 Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#127} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [add] https://crrev.com/7336b1875146492d1c7c41c4bcf63e7f851b5410/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.invalid-code-cache.html [add] https://crrev.com/7336b1875146492d1c7c41c4bcf63e7f851b5410/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/invalid-code-cache-worker.js [modify] https://crrev.com/7336b1875146492d1c7c41c4bcf63e7f851b5410/third_party/blink/renderer/bindings/core/v8/v8_script_runner.cc
,
Apr 19 2018
I can also confirm that Chrome Version 68.0.3400.0 (Official Build) canary (64-bit) no longer gives errors while using cache.add() with ES Modules, while opening https://swesmit.hwalab.com/ (also with chrome://flags/#enable-pwa-full-code-cache enabled). |
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by krajshree@chromium.org
, Apr 13 2018