Closed Bug 859685 Opened 10 years ago Closed 9 years ago

lazyAppend in VariableViews.jsm does a DISPATCH_SYNC from the main thread (spins the event loop)

Categories

(DevTools :: Object Inspector, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 830344

People

(Reporter: jesup, Unassigned)

Details

In VariableViews.jsm:1546, it does:
      // Allow for a paint flush.
      Services.tm.currentThread.dispatch({ run: this._batchAppend }, 1);

(Note that '1' should be Ci.nsIThread.DISPATCH_SYNC... and other uses in debugger that use 0 should be Ci.nsIThread.DISPATCH_NORMAL)

This happens on the main thread when testing browser/devtools/debugger/test via mach.

DISPATCH_SYNC from the MainThread can cause reentrancy in JS, among other things, and generally is unsafe (and likely is here as well).
This was intended. Ci.nsIThread.DISPATCH_NORMAL performed very (very!) poorly in that specific case. I can only guess the actual reason, but DISPATCH_SYNC worked much better so that's what was used instead.

Is there any alternative that should be relied upon?
To compare, run the test browser_dbg_propertyview-data-big.js with DISPATCH_NORMAL vs. DISPATCH_SYNC.
The problem (if I'm correct) is that khuey and others have stated that DISPATCH_SYNC from mainthread is never safe.  So, running faster isn't the issue.  You'll have to find another way than DISPATCH_SYNC.  For example, decompose the function into stuff that happens before dispatch(), and stuff that happens after, and put the "after" stuff in a runnable that gets sent back to mainthread when the (async) runnable finishes on the target thread.  In c++ (there are a few ways to do this):

 Run()
 {
   if (on_target) {
     do_whatever;
     on_target = false;
     mainthread->Dispatch(this,DISPATCH_NORMAL);
     // or if you must, create a new runnable, but that means an allocation
   } else {
     do_operation_is_complete();
   }
   return NS_OK; 
 }

If in this case it's actually safe once bsmedberg/bz/khuey look:
a) change to nsIThread.DISPATCH_SYNC (or NORMAL)
b) comment why it's safe
Let's step back for a second.  Let's ignore whether or not this is safe, because the debugger involves nested event loops out of necessity.  Instead let's address comment 1.

(In reply to Victor Porof [:vp] from comment #1)
> This was intended. Ci.nsIThread.DISPATCH_NORMAL performed very (very!)
> poorly in that specific case. I can only guess the actual reason, but
> DISPATCH_SYNC worked much better so that's what was used instead.

The reason DISPATCH_NORMAL performs poorly here is because the debugger is hogging the event loop.  It doesn't matter if you batch up these appends if something is just calling append in a giant loop.  DISPATCH_SYNC solves your problem not because it runs the append synchronously, but because it spins the event loop and allows other events to be processed.

Manually spinning the event loop is a pretty terrible way to yield to other tasks.  The debugger needs to return the original event loop (which is really the nested event lopp that it spins up to block the running JS) and allow it to process events.
Priority: -- → P3
Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.
Component: Developer Tools: Debugger → Developer Tools: Object Inspector
This will be removed in bug 830344.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 830344
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.