Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 6 years ago

The following testcase crashes on mozilla-central revision 2872e7a3606d (build with --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/tests/test262/language/module-code/dynamic-import/syntax/valid/nested-block-empty-str-is-valid-assign-expr.js
import("");

Backtrace:

#0  0x0000559dff27d9c5 in JSContext::getPendingException (this=0x7f429e618000, rval=...) at js/src/vm/JSContext.cpp:1449
#1  0x0000559dfeadbece in js::GetAndClearException (cx=0x7f429e618000, res=...) at js/src/vm/Interpreter.cpp:5077
#2  0x0000559dfeb6277d in js::RejectPromiseWithPendingError (cx=<optimized out>, promise=...) at js/src/builtin/Promise.cpp:3412
#3  js::StartDynamicModuleImport (cx=0x7f429e618000, referencingPrivate=..., specifierArg=...) at js/src/builtin/ModuleObject.cpp:1849
#4  0x0000559dfead4bd7 in Interpret (cx=0x7f429e618000, state=...) at js/src/vm/Interpreter.cpp:4737
#5  0x0000559dfeac3a93 in js::RunScript (cx=0x7f429e618000, state=...) at js/src/vm/Interpreter.cpp:447
/snip

For detailed crash information, see attachment.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 2 • 6 years ago

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/41812db6caba
user:        Jon Coppeard
date:        Mon Oct 22 11:28:17 2018 +0100
summary:     Bug 1499140 - Support dynamic module import in the shell r=jandem

Jon, is bug 1499140 a likely regressor?

	Jon Coppeard (:jonco) Assignee
	Comment 3 • 6 years ago

There's a couple of things going on here.

The first is that we accept the empty string as a valid module specifier.  For HTML this has to be a URL or a relative path.  For the shell we can do whatever we want but "" clearly should be rejected.

The second is that the shell's FileAsString function to read the contents of a file doesn't cope well with errors.  

What's happening for this testcase is that we end up resolving the module's path to a directory and attempting to read that.  The call to ftell() fails but the return value is not checked.  In this case it returns -1 which we use as the length to read.  When allocation fails we don't throw an exception which causes this assertion.

The patch disallows the empty string as a module specifier and also tidies up the error handling in FileAsString.

	Pulsebot
	Comment 4 • 6 years ago

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6a67ed98c7
Disallow the empty string as a module specifier in the shell and improve error handling in js::shell::FileAsString() r=bbouvier

	Stefan Hindli [:stefan_hindli]
	Comment 5 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6f6a67ed98c7