1283401 - CacheStorage: assertion failure when CacheStorage.keys() returns error

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jarda Snajdr [:jsnajdr]]

Steps to reproduce:
1. Open debug build of Firefox (more MOZ_ASSERTs are there and failures crash)
2. Go to about:home
3. Open devtools on the page, open Storage Inspector (you might need to enable it first in options, it's hidden by default)
4. Storage Inspector will try to access CacheStorage for about:home
5. CacheStorage is not supported on about: URLs, so while calling CacheStorage.keys(), NS_ERROR_DOM_BAD_URI is thrown

Unfortunately, there is also assertion failure in CacheOpParent::OnPrincipalVerified:

Assertion failure: !Failed(), at /Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/include/mozilla/ErrorResult.h:105
#01: mozilla::dom::cache::PrincipalVerifier::CompleteOnInitiatingThread()[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x21de34a]
#02: mozilla::dom::cache::PrincipalVerifier::Run()[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x21ddc51]
[27221] WARNING: 'aRv.Failed()', file /Users/jsnajdr/src/gecko-dev/dom/cache/CacheOpChild.cpp, line 108
#03: nsThread::ProcessNextEvent(bool, bool*)[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xf4abd]
#04: NS_ProcessNextEvent(nsIThread*, bool)[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x134b73]
#05: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x66d312]
#06: MessageLoop::Run()[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x62d89c]
#07: nsThread::ThreadFunc(void*)[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xf24c6]
#08: _pt_root[/Users/jsnajdr/src/gecko-dev/obj-x86_64-apple-darwin15.5.0/dist/NightlyDebug.app/Contents/MacOS/libnss3.dylib +0x2eda20]
#09: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x399d]
#10: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x391a]

It happens because the ErrorResult destructor checks if the error was handled somehow [1], but in this case, CacheOpParent::Send__delete__ just sends the error over IPDL to the child and the ErrorResult is left intact [2, 3].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bindings/ErrorResult.h#105
[2] https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheOpParent.cpp#148
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bindings/ErrorIPCUtils.h#28-51

Comment 1

[Jarda Snajdr [:jsnajdr]]

Attachment: CacheStorage: assertion failure when CacheStorage.keys() returns error

This patch calls ErrorResult::SuppressException in CacheOpParent::OnPrincipalVerified and prevents the assert failure.

Another way how to fix this is to call SuppressException in the Write method at [1]. This looks like a better and more general way to fix this, but has a potential to break many things: calling SuppressException clears the error code back to NS_OK, and if someone tries to get the error code after calling Write(), the error is no longer there. I need advice here whether it's safe or not.

Then I'll write an appropriate test. I'm a bit surprised that dom/cache/test/mochitest/test_cache_untrusted.html is not already failing in debug builds - maybe because it doesn't call caches.keys(), but only caches.open()?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8766706 [details] [diff] [review]
CacheStorage: assertion failure when CacheStorage.keys() returns error

Review of attachment 8766706 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks!  This is consistent with our usage here:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/CacheOpParent.cpp#170

Comment 3

[Jarda Snajdr [:jsnajdr]]

Attachment: CacheStorage: assertion failure when CacheStorage.keys() returns error

Alright, I updated the patch with a fix to few other locations where ErrorResult is sent over IPDL.

Test coverage will be provided by bug 1283800 (devtools storage inspector recovering from failure to access localStorage).

Without this patch, the test in bug 1283800 crashes in debug builds, as expected:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d80fccaefce7

With this patch, it starts to pass:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=252a86f56e46

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8767159 [details] [diff] [review]
CacheStorage: assertion failure when CacheStorage.keys() returns error

Review of attachment 8767159 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/99bab2d812fc
CacheStorage: assertion failure when CacheStorage.keys() returns error r=bkelly

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99bab2d812fc