1213526 - Cache can invoke scriptable nsIURI::GetAsciiSpec (which may be implemented by XPConnect-backed JS) off the main thread

Status: NEW

(Core :: Networking: Cache, defect, P5)

Description

[Andrew Sutherland [:asuth] (he/him)]

Horrible context: The FxOS gaia email app has a test runner that currently uses b2g-desktop in xulrunner mode with a custom JS-implemented protocol and JS-implemented URI implementation (stolen from jetpack/add-on sdk stuff) so that we can have URIs like "testfile://test_imap_general-imap_fake/test/loggest-runner.html" that are able to appear as installed apps and also give each test their own origin.

I just tried to run the test runner under mulet and encountered a segfault where CacheEntry::HashingKey(...nsIURI...) was invoked off the main thread.  In this function, aURI->GetAsciiSpec(spec) is invoked (https://dxr.mozilla.org/mozilla-central/source/netwerk/cache2/CacheEntry.cpp#228).  Since nsIURI is marked as scriptable, it seems inappropriate to invoke such things off the main thread, despite the many dubious shenanigans happening in my code.  (NB: I am seriously considering moving to a more mochitest-based model where we just use an http server to avoid the implementation horrors I have wrought.)

Here's backtrace info from a mozilla-download downloaded nightly build of mulet; if there's interest I can run a local build to get more detailed symbol info, etc.:

Thread 0x7ffff6b33700 (LWP 14606) "Cache2 I/O":

  0 nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*):0
  1 PrepareAndDispatch:0
  2 SharedStub:0
  3 net::CacheEntry::HashingKey(nsACString_internal const&, nsACString_internal const&, nsIURI*, nsACString_internal&):0
  4 net::(anonymous namespace)::TelemetryEntryKey(mozilla::net::CacheEntry const*, nsAutoCString&):0
  5 net::CacheStorageService::TelemetryRecordEntryCreation(mozilla::net::CacheEntry const*):0
  6 net::CacheStorageService::RegisterEntry(mozilla::net::CacheEntry*):0
  7 net::CacheEntry::BackgroundOp(unsigned int, bool):0
  8 net::CacheEntry::Run():0
  9 net::CacheIOThread::LoopOneLevel(unsigned int):0
 10 net::CacheIOThread::ThreadFunc():0
 11 net::CacheIOThread::ThreadFunc(void*):0
 12 _pt_root:0
 13 start_thread:333
 14 clone:109

Comment 1

[Honza Bambas (:mayhemer)]

Can you use simple URI (nsSimpleURI) instead?

The cache code could potentially be converted to use something else than a URI internally, tho that's something I would like to avoid.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #1)
> Can you use simple URI (nsSimpleURI) instead?

Yes, I've switched to nsStandardURL now.

> The cache code could potentially be converted to use something else than a
> URI internally, tho that's something I would like to avoid.

It does seem like nsIURI is a fundamental building block that should be something that can be accessed off the main thread.  But I think that means that its interface should be marked "builtinclass" given that "scriptable" without "builtinclass" means it's able to be implemented as a main-thread-only-XPConnect-powered-JS.  Should I file a bug for adding builtinclass to nsIURI/nsIURL?

The existing in-tree JS implementations of nsIURI seem to mainly be copied-and-pasted unit tests of the http-index-format stream converter that themselves could be converted to using nsStandardURL.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Andrew Sutherland [:asuth] from comment #2)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #1)
> > Can you use simple URI (nsSimpleURI) instead?
> 
> Yes, I've switched to nsStandardURL now.
> 
> > The cache code could potentially be converted to use something else than a
> > URI internally, tho that's something I would like to avoid.
> 
> It does seem like nsIURI is a fundamental building block that should be
> something that can be accessed off the main thread.  But I think that means
> that its interface should be marked "builtinclass" given that "scriptable"
> without "builtinclass" means it's able to be implemented as a
> main-thread-only-XPConnect-powered-JS.  Should I file a bug for adding
> builtinclass to nsIURI/nsIURL?

There are already some plans on making our URL impl threadsafe, so, no, don't file a bug.  I would have to look around for any existing bug #s tho.

> 
> The existing in-tree JS implementations of nsIURI seem to mainly be
> copied-and-pasted unit tests of the http-index-format stream converter that
> themselves could be converted to using nsStandardURL.

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258