Closed Bug 1452114 Opened 2 years ago Closed 2 years ago

Spurious report of OutOfMemory if a script fails to parse

Categories

(Core :: JavaScript Engine, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jesup, Assigned: jandem)

References

(Depends on 1 open bug)

Details

Attachments

(4 files)

I have been seeing spurious out-of-memory reports in the browser console and to stderr... they're coming from google calendar (in a pinned tab), when a client.js script fails to parse properly.

It appears to come from HelperThread.cpp:finishParseTask() around line 1595:
    if (!script) {
        // No error was reported, but no script produced. Assume we hit out of
        // memory.
        ReportOutOfMemory(cx);

Reports from stderr:
JavaScript error: https://0.client-channel.google.com/client-channel/client?cfg=%7B%222%22%3A%22calendar%22%2C%228%22%3Afalse%2C%2213%22%3Afalse%7D&ctype=calendar&xpc=%7B%22cn%22%3A%22zcINELaGeX%22%2C%22tp%22%3A1%2C%22osh%22%3Anull%2C%22ppu%22%3A%22https%3A%2F%2Fcalendar.google.com%2Frobots.txt%22%2C%22lpu%22%3A%22https%3A%2F%2F0.client-channel.google.com%2Frobots.txt%22%7D, line 4: ReferenceError: buzz is not defined
JavaScript error: , line 0: uncaught exception: out of memory


JavaScript error: https://clients4.google.com/invalidation/lcs/client?xpc=%7B%22cn%22%3A%22jKJggnFean%22%2C%22tp%22%3A1%2C%22osh%22%3Anull%2C%22ppu%22%3A%22https%3A%2F%2Fhangouts.google.com%2Frobots.txt%22%2C%22lpu%22%3A%22https%3A%2F%2Fclients4.google.com%2Frobots.txt%22%7D, line 2: ReferenceError: invalidation is not defined
JavaScript error: , line 0: uncaught exception: out of memory


JavaScript error: https://clients4.google.com/invalidation/lcs/client?xpc=%7B%22cn%22%3A%22VV6MdqRzk7%22%2C%22tp%22%3A1%2C%22osh%22%3Anull%2C%22ppu%22%3A%22https%3A%2F%2Fcalendar.google.com%2Frobots.txt%22%2C%22lpu%22%3A%22https%3A%2F%2Fclients4.google.com%2Frobots.txt%22%7D, line 2: ReferenceError: invalidation is not defined
JavaScript error: , line 0: uncaught exception: out of memory


One script looks like:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"><html><head><script type="text/javascript" src="/client-channel/js/805552573-lcs_client_bin.js"></script> <script type="text/javascript" src="https://apis.google.com/js/api.js"></script></head> <body><script type="text/javascript">
            (function() {
            
              new buzz.channel.IframedBrowserChannelServer(
                 "/client-channel/channel", "{\x222\x22:\x22hangouts\x22,\x226\x22:\x22gmail\x22,\x227\x22:\x22chat_frontend_20180402.12_p0\x22,\x228\x22:false,\x229\x22:\x22/client-channel/gsid\x22,\x2211\x22:5000,\x2212\x22:180,\x2213\x22:false,\x2214\x22:45000}",
                 
                   new buzz.channel.XpcHostPageChannel("{\x22lpu\x22:\x22https://0.client-channel.google.com/robots.txt\x22,\x22ppu\x22:\x22https://hangouts.google.com/robots.txt\x22,\x22osh\x22:null,\x22cn\x22:\x22PsEFfTLk9j\x22,\x22tp\x22:1}")
                 ).init(0,
                                 "/client-channel/gsid");
            
            })();
          </script></body></html>

Another looks like:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"><html><head><script type="text/javascript" src="/invalidation/lcs/js/2543049534-lcs_receiver_bin.js"></script></head> <body><script type="text/javascript">
            new invalidation.XpcSenderServer("/invalidation/lcs/request",
                '0', null).init();
          </script></body></html>
Flags: needinfo?(jdemooij)
I can't reproduce this, but I can do a Try push with an assert there to see i that reveals something...
(In reply to Jan de Mooij [:jandem] from comment #1)
> I can't reproduce this, but I can do a Try push with an assert there to see
> i that reveals something...

This came back green [1], so I'm not sure what to do. Maybe we could add (release) asserts..

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f716a1b12a8416ba7231e434cfe4578bbbef67&group_state=expanded
Flags: needinfo?(jdemooij)
> It appears to come from HelperThread.cpp:finishParseTask() around line 1595:
>     if (!script) {
>         // No error was reported, but no script produced. Assume we hit out of
>         // memory.
>         ReportOutOfMemory(cx);

jesup, what makes you suspect this code?

Jan, if we can release-assert that this never happens, then we definitely should. Spurious errors are bad, obviously.
Flags: needinfo?(rjesup)
Flags: needinfo?(jdemooij)
Priority: -- → P2
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> > It appears to come from HelperThread.cpp:finishParseTask() around line 1595:
> >     if (!script) {
> >         // No error was reported, but no script produced. Assume we hit out of
> >         // memory.
> >         ReportOutOfMemory(cx);
> 
> jesup, what makes you suspect this code?

I think I tracked it down to exactly there.  Other conversations after this was filed indicated it might have been a bug in alt-data - a number of pages had non-working scripts.  shift-reload cleared them up, which tends to back that up.  So that may be *why* we had a failure in the parsetask.
 
> Jan, if we can release-assert that this never happens, then we definitely
> should. Spurious errors are bad, obviously.

Not sure this should be release-assert if bad alt-data/etc can trigger it!  NS_ASSERTION to orange any tests, however?
Flags: needinfo?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #4)
> Not sure this should be release-assert if bad alt-data/etc can trigger it! 
> NS_ASSERTION to orange any tests, however?

We don't have NS_ASSERTION in JS (fatal asserts FTW). I added a MOZ_ASSERT but it didn't hit on any tests on Try.

One option is to throw fuzzing at it first, I can post a patch for that.
decoder, gkw: sorry for bothering you guys again :( :jesup hit a spurious OOM exception in the browser and we can't reproduce it. This patch adds 2 release asserts, if the fuzzers hit one of them that would be interesting.
Flags: needinfo?(jdemooij)
Attachment #8967295 - Flags: feedback?(nth10sd)
Attachment #8967295 - Flags: feedback?(choller)
Comment on attachment 8967295 [details] [diff] [review]
Patch for fuzzing

I did find something, testcase coming up.

Thread 1 "js-64-linux-145" received signal SIGSEGV, Segmentation fault.
js::GlobalHelperThreadState::finishParseTask (this=0x7ffff5f04400, cx=0x7ffff5f14000, kind=<optimized out>, token=<optimized out>)
    at /home/ubuntu/trees/mozilla-central/js/src/vm/HelperThreads.cpp:1594
warning: Source file is more recent than executable.
1594        if (!script) {
(gdb) bt
#0  js::GlobalHelperThreadState::finishParseTask (this=0x7ffff5f04400, cx=0x7ffff5f14000, kind=<optimized out>, token=<optimized out>)
    at /home/ubuntu/trees/mozilla-central/js/src/vm/HelperThreads.cpp:1594
#1  0x000000000044b8e3 in runOffThreadScript (cx=0x7ffff5f14000, argc=0, vp=0x7fffffffd138) at /home/ubuntu/trees/mozilla-central/js/src/shell/js.cpp:4564
#2  0x0000000000521202 in js::CallJSNative (args=..., native=0x44b810 <runOffThreadScript(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff5f14000)
    at /home/ubuntu/trees/mozilla-central/js/src/vm/JSContext-inl.h:280
#3  js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f14000, args=..., construct=construct@entry=js::NO_CONSTRUCT)
    at /home/ubuntu/trees/mozilla-central/js/src/vm/Interpreter.cpp:467
#4  0x0000000000523218 in InternalCall (args=..., cx=0x7ffff5f14000) at /home/ubuntu/trees/mozilla-central/js/src/vm/Interpreter.cpp:516
#5  js::CallFromStack (cx=cx@entry=0xffffdd00, args=...) at /home/ubuntu/trees/mozilla-central/js/src/vm/Interpreter.cpp:522
#6  0x00000000005c25d5 in js::jit::DoCallFallback (cx=0xffffdd00, frame=0x7fffffffd178, stub_=0x7ffff3177e90, argc=0, vp=0x7fffffffd138, res=...)
    at /home/ubuntu/trees/mozilla-central/js/src/jit/BaselineIC.cpp:2380
#7  0x00000f1554d04e81 in ?? ()
#8  0x0000000000000000 in ?? ()
(gdb)

Applied the patch off m-c rev 7ff499dfcd51, used a 64-bit non-deterministic opt build.
Attachment #8967295 - Flags: feedback?(nth10sd)
Attached file testcase
Run:

--fuzzing-safe --ion-offthread-compile=off --baseline-eager 2810819.js
Flags: needinfo?(jdemooij)
Attachment #8967295 - Flags: feedback?(choller)
This fixes an OOM issue the fuzzers found and I could repro locally: in GCRuntime::tryNewTenuredThing, we also want to report OOM if we're on a helper thread.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8970114 - Flags: review?(jcoppeard)
Comment on attachment 8970114 [details] [diff] [review]
Part 1 - Fix tryNewTenuredThing OOM bug

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

Ah, nice find.  Thanks for fixing this.
Attachment #8970114 - Flags: review?(jcoppeard) → review+
This adds diagnostic asserts. Considering fuzzing and Try didn't blow up it's probably safe to do this... Maybe later we can upgrade this to a release assert.
Attachment #8970846 - Flags: review?(jcoppeard)
Attachment #8970846 - Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e599141a1af3
part 1 - Fix GCRuntime::tryNewTenuredThing to report OOM on helper threads as well. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/92824bef6153
part 2 - Add diagnostic asserts. r=jonco
https://hg.mozilla.org/mozilla-central/rev/e599141a1af3
https://hg.mozilla.org/mozilla-central/rev/92824bef6153
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1458209
You need to log in before you can comment on or make changes to this bug.