1322533 - busy loop of inserting elements that fire load events that insert more elements hangs/crashes Firefox (no slow script dialog)

Status: UNCONFIRMED

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

Description

[Markus Keller]

Attachment: crash.html -- blocks/crashes FF 50.0.2

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161129173726

Steps to reproduce:

Open the attached crash.html on Windows 7.

document body:

  Crashing...
  <p>
    <img src="https://bugzilla.mozilla.org/skins/contrib/Mozilla/bugzilla-person-alternate.png" alt="" />
  </p>

  <script type="text/javascript">
    document.addEventListener('load', function doit() {
      var css = document.createElement("style");
      document.head.appendChild(css);
    }, true);
  </script>


Actual results:

In my main profile (about 15 windows with 50+ tabs), FF was first blocked, and then it crashed.

In a new profile, it only blocked forever. Windows fades the window and appends (Not Responding) to the title.


Expected results:

Firefox should stay responsive and let me close the window.

I was writing a page, and this is a minimal example that crashed. I don't expect it to do anything useful.

Feel free to uncheck this bug's Security flag if normal crashes are generally made public.

Comment 1

[:Gijs (he/him)]

style tags fire a load event, so your code contains an infinite loop. Not sure why we don't show the slow script dialog, but this doesn't need to stay security-sensitive, no.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I wonder if we could track the current event handler depth and yield the thread if we hit some limit.

Comment 3

[Boris Zbarsky [:bzbarsky]]

That wouldn't help, because we don't have recursion here.  What we do have is the nasty hackery in SheetLoadData::ScheduleLoadEventIfNeeded in layout/style/Loader.cpp, where we try to dispatch the load event "as soon as possible, but async" via the thread observer machinery.  It sounds like we're effectively completely swamping the event loop, right?

I guess this is basically like having a promise handler that resolves another promise (and hence queues having its handler get called), in terms of how the event loop behaves?  What's a bit unexpected is the crash bit.  Unless OnProcessNextEvent/AfterProcessNextEvent can cause recursion somehow?

Anyway, I would be really happy to remove this hackery from the style loader and replace it with a microtask or something.  Or even just a runnable, but then we should think about skipping the extra runnable in the async load case or something, to address sicking's concerns about layout happening with the new style data before the load event fires.  If you insert an inline stylesheet (as in this testcase), you would still be SOL and could get layout happening before load fires.  :(

Comment 4

[Andrew Overholt [:overholt]]

If you have a victim in mind here, Boris, let me know.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Um... "sicking"?  I know that's not very helpful...

If we have someone who wants to learn more than they ever wanted to know about the event loop _and_ the CSS loader, this is the bug for them.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information