Closed Bug 1368992 Opened 3 years ago Closed 3 years ago

Assertion in mozilla::dom::ScriptLoader::RegisterForBytecodeEncoding

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: michal, Assigned: nbp)

References

Details

Attachments

(1 file)

Program received signal SIGSEGV, Segmentation fault.
mozilla::dom::ScriptLoader::RegisterForBytecodeEncoding (this=0x85ea27e0, aRequest=0x84e84600)
    at /opt/moz/hg-inbound-3/dom/script/ScriptLoader.cpp:2046
2046	  MOZ_ASSERT(aRequest->mScript);
(gdb) bt 10
#0  0xffffffff in mozilla::dom::ScriptLoader::RegisterForBytecodeEncoding(mozilla::dom::ScriptLoadRequest*) (this=0x85ea27e0, aRequest=0x84e84600) at /opt/moz/hg-inbound-3/dom/script/ScriptLoader.cpp:2046
#1  0xffffffff in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) (this=0x85ea27e0, aRequest=0x84e84600) at /opt/moz/hg-inbound-3/dom/script/ScriptLoader.cpp:2021
#2  0xffffffff in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) (this=0x85ea27e0, aRequest=0x84e84600) at /opt/moz/hg-inbound-3/dom/script/ScriptLoader.cpp:1753
#3  0xffffffff in mozilla::dom::ScriptLoader::ProcessPendingRequests() (this=0x85ea27e0)
    at /opt/moz/hg-inbound-3/dom/script/ScriptLoader.cpp:2246
#4  0xffffffff in mozilla::detail::RunnableMethodImpl<mozilla::dom::ScriptLoader*, void (mozilla::dom::ScriptLoader::*)(), true, (mozilla::RunnableKind)0>::Run() (args=..., m=<optimized out>, o=<optimized out>)
    at /opt/moz/hg-inbound-3/_obj-browser-release-tb-fp-dbg/dist/include/nsThreadUtils.h:1084
#5  0xffffffff in mozilla::detail::RunnableMethodImpl<mozilla::dom::ScriptLoader*, void (mozilla::dom::ScriptLoader::*)(), true, (mozilla::RunnableKind)0>::Run() (m=<optimized out>, o=<optimized out>, this=0x836d6ae0)
    at /opt/moz/hg-inbound-3/_obj-browser-release-tb-fp-dbg/dist/include/nsThreadUtils.h:1091
#6  0xffffffff in mozilla::detail::RunnableMethodImpl<mozilla::dom::ScriptLoader*, void (mozilla::dom::ScriptLoader::*)(), true, (mozilla::RunnableKind)0>::Run() (this=0x836d6ac0)
    at /opt/moz/hg-inbound-3/_obj-browser-release-tb-fp-dbg/dist/include/nsThreadUtils.h:1133
#7  0xffffffff in nsThread::ProcessNextEvent(bool, bool*) (this=0xf3521140, aMayWait=false, aResult=0xffed436f)
    at /opt/moz/hg-inbound-3/xpcom/threads/nsThread.cpp:1322
#8  0xffffffff in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0xf3521140, aMayWait=false)
    at /opt/moz/hg-inbound-3/xpcom/threads/nsThreadUtils.cpp:472
#9  0xffffffff in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0xf71f22b0, aDelegate=0xf7137440)
    at /opt/moz/hg-inbound-3/ipc/glue/MessagePump.cpp:96
#10 0xffffffff in MessageLoop::RunInternal() (this=0xf7137440) at /opt/moz/hg-inbound-3/ipc/chromium/src/base/message_loop.cc:238
This assertion can happen if mScript is nullptr, which would happen if the result of the parser failed to generate a script, due to syntax error for example.

The NS_SUCCEEDED condition which is guarding the call to RegisterForBytecodeEncoding is supposed to prevent such errors, but in case of compilation error, we generate the error code with EvaluationExceptionToNSResult (nsJSUtils.cpp) which only produces NS_SUCCESS_* results.

I will make a test case and fix the condition.
Thanks for opening the bug.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #8872969 - Flags: review?(mrbkap)
Comment on attachment 8872969 [details] [diff] [review]
JS bytecode cache: Do not attempt to encode bytecode if the compilation failed.

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

r=me with my comment addressed.

::: dom/script/ScriptLoader.cpp
@@ +2144,5 @@
>              }
>            }
>  
>            // Queue the current script load request to later save the bytecode.
> +          if (script && encodeBytecode) {

What's the purpose of the `rv` returned by CompileAndExec? Should we be checking `NS_SUCCEEDED(rv) && script && encodeBytecode`?
Attachment #8872969 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #3)
> ::: dom/script/ScriptLoader.cpp
> @@ +2144,5 @@
> >              }
> >            }
> >  
> >            // Queue the current script load request to later save the bytecode.
> > +          if (script && encodeBytecode) {
> 
> What's the purpose of the `rv` returned by CompileAndExec? Should we be
> checking `NS_SUCCEEDED(rv) && script && encodeBytecode`?

For the moment, the rv value are either "threw" or "threw-uncatchable", which are nsresult success values [1]. The interesting part is if we managed to generate a JSScript [2] at the end of the compilation.  The fact that we manage or not to execute the code is almost not relevant.

As far as I can see, the only use of "rv" is to bubble up as a returned value and to be given as argument of ScriptEvaluated [3].

[1] http://searchfox.org/mozilla-central/source/dom/base/nsJSUtils.cpp#130,132
[2] http://searchfox.org/mozilla-central/source/dom/base/nsJSUtils.cpp#253
[3] http://searchfox.org/mozilla-central/source/dom/script/ScriptElement.cpp#56
I will also note, that we should not check for NS_SUCCEEDED(rv), because if both (script && encodeBytecode) then we might have started encoding the bytecode for the given script.  By moving the ScriptLoadRequest to the list of script that we are watching for finalizing the encoding, we will properly terminate any started encoding.

I will also note, that if we fail at starting the encoding, then we might still attempt to call the function to finish the encoding, which would result in an Assertion failure / SEGV at (null) at [1].  I will open a follow-up bug to fix that.

[1] http://searchfox.org/mozilla-central/source/js/src/jsscript.cpp#2068,2073
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0148cb8a7272
JS bytecode cache: Do not attempt to encode bytecode if the compilation failed. r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/0148cb8a7272
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.