Bug 60790 – Make DocumentEventQueue spawn a Task for each asynchronous event

• Component: New Bugs
• Status: RESOLVED WONTFIX

[David Grogan]

Make EventQueue spawn a Task for each asynchronous event

[David Grogan]

Created attachment 93496 [details]
Patch

[David Grogan]

Created attachment 93497 [details]
Patch

[David Grogan]

Dave, could you take a look at this patch when you get a chance?

This is the smallest change extracted from bug 57789.  I fear I butchered object-lifetime management; I don't know how EventQueue, Events and Tasks get de-allocated.

[David Levin]

Comment on attachment 93497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93497&action=review

> Source/WebCore/dom/EventQueue.cpp:51
> +    : m_scriptExecutionContext(context)

How do you know that EventQueue won't outlive context?

> Source/WebCore/dom/EventQueue.cpp:90
> +      : m_event(event)

4 space indent

> Source/WebCore/dom/EventQueue.cpp:94
> +    }

Add blank line.

> Source/WebCore/dom/EventQueue.cpp:96
> +    EventQueue* m_eventQueue;

Should this be a ref ptr? How do you know that m_eventQueue will still be alive when this runs?

> Source/WebCore/dom/EventQueue.cpp:108
> +        m_eventTaskMap.remove(event);

You should be able to call remove alone and not even do contains.

> Source/WebCore/dom/EventQueue.cpp:139
> +    bool found = m_eventTaskMap.contains(event);

Why not just use get directly and compare the result to 0?

Or you could use "take" and then just avoid calling "removeEvent" when the event is cancelled.

> Source/WebCore/dom/EventQueue.cpp:140
> +    if (found) {

Perfer return early.

If (!found)
    return false;

> Source/WebCore/dom/EventQueue.cpp:149
> +    m_eventTaskMap.clear();

Don't you need to iterate through this map and call cancel on each task?

[David Grogan]

Comment on attachment 93497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93497&action=review

>> Source/WebCore/dom/EventQueue.cpp:51
>> +    : m_scriptExecutionContext(context)
> 
> How do you know that EventQueue won't outlive context?

The only instantiator of EventQueue is Document.  Document only exposes a raw EventQueue pointer to clients, not a PassRefPtr.  So EventQueue should only be destructed when Context is destructed.  Is that accurate?

>> Source/WebCore/dom/EventQueue.cpp:96
>> +    EventQueue* m_eventQueue;
> 
> Should this be a ref ptr? How do you know that m_eventQueue will still be alive when this runs?

I'm not sure and I don't, respectively :(
This is the kind of thing I was referring to in comment 3 with "I fear I butchered object-lifetime management; I don't know how EventQueue, Events and Tasks get de-allocated."

I _think_ keeping EventQueue as a raw pointer should be ok if EventDispatcherTask exposes a flag-setting method, e.g. "eventQueueIsDead", that EventQueue's destructor calls on each of its tasks.  The task would check the flag before calling m_eventQueue->removeEvent.  Would that work?

Is there a test that you know of that would have exposed this potential null pointer dereference?  I've just been running the layout tests.

>> Source/WebCore/dom/EventQueue.cpp:149
>> +    m_eventTaskMap.clear();
> 
> Don't you need to iterate through this map and call cancel on each task?

Yes, that would be better.  I'll add that.  Currently, I don't think it matters in practice because canceledQueuedEvents is only called in Document::detach, after stopActiveDOMObjects is called.  But you're right, cancelling each task is more true to the function name.

[David Grogan]

(In reply to comment #5)
> (From update of attachment 93497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93497&action=review
> 
> >> Source/WebCore/dom/EventQueue.cpp:51
> >> +    : m_scriptExecutionContext(context)
> > 
> > How do you know that EventQueue won't outlive context?
> 
> The only instantiator of EventQueue is Document.  Document only exposes a raw EventQueue pointer to clients, not a PassRefPtr.  So EventQueue should only be destructed when Context is destructed.  Is that accurate?

"Context" should be "Document", sorry.

[David Levin]

(In reply to comment #5)
> (From update of attachment 93497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93497&action=review
> 
> >> Source/WebCore/dom/EventQueue.cpp:51
> >> +    : m_scriptExecutionContext(context)
> > 
> > How do you know that EventQueue won't outlive context?
> 
> The only instantiator of EventQueue is Document.  Document only exposes a raw EventQueue pointer to clients, not a PassRefPtr.  So EventQueue should only be destructed when Context is destructed.  Is that accurate?
> 
> >> Source/WebCore/dom/EventQueue.cpp:96
> >> +    EventQueue* m_eventQueue;
> > 
> > Should this be a ref ptr? How do you know that m_eventQueue will still be alive when this runs?
> 
> I'm not sure and I don't, respectively :(
> This is the kind of thing I was referring to in comment 3 with "I fear I butchered object-lifetime management; I don't know how EventQueue, Events and Tasks get de-allocated."
> 
> I _think_ keeping EventQueue as a raw pointer should be ok if EventDispatcherTask exposes a flag-setting method, e.g. "eventQueueIsDead", that EventQueue's destructor calls on each of its tasks.  The task would check the flag before calling m_eventQueue->removeEvent.  Would that work?
> 
> Is there a test that you know of that would have exposed this potential null pointer dereference?  I've just been running the layout tests.


If EventQueue cancels all tasks when it dies and you do a change to remove tasks from the map when they are cancelled. Then when a task is in the cancelled state, it can just return and not even access EventQueue.

That should solve the issue.

[David Grogan]

Comment on attachment 93497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93497&action=review

>>>>> Source/WebCore/dom/EventQueue.cpp:51
>>>>> +    : m_scriptExecutionContext(context)
>>>> 
>>>> How do you know that EventQueue won't outlive context?
>>> 
>>> The only instantiator of EventQueue is Document.  Document only exposes a raw EventQueue pointer to clients, not a PassRefPtr.  So EventQueue should only be destructed when Context is destructed.  Is that accurate?
>> 
>> "Context" should be "Document", sorry.
> 
> If EventQueue cancels all tasks when it dies and you do a change to remove tasks from the map when they are cancelled. Then when a task is in the cancelled state, it can just return and not even access EventQueue.
> 
> That should solve the issue.

Ah, yes.  That is pretty straightforward.

>> Source/WebCore/dom/EventQueue.cpp:90
>> +      : m_event(event)
> 
> 4 space indent

done

>> Source/WebCore/dom/EventQueue.cpp:94
>> +    }
> 
> Add blank line.

done

>>> Source/WebCore/dom/EventQueue.cpp:96
>>> +    EventQueue* m_eventQueue;
>> 
>> Should this be a ref ptr? How do you know that m_eventQueue will still be alive when this runs?
> 
> I'm not sure and I don't, respectively :(
> This is the kind of thing I was referring to in comment 3 with "I fear I butchered object-lifetime management; I don't know how EventQueue, Events and Tasks get de-allocated."
> 
> I _think_ keeping EventQueue as a raw pointer should be ok if EventDispatcherTask exposes a flag-setting method, e.g. "eventQueueIsDead", that EventQueue's destructor calls on each of its tasks.  The task would check the flag before calling m_eventQueue->removeEvent.  Would that work?
> 
> Is there a test that you know of that would have exposed this potential null pointer dereference?  I've just been running the layout tests.

Do we have existing types of tests that would catch object-lifetime problems?

>> Source/WebCore/dom/EventQueue.cpp:108
>> +        m_eventTaskMap.remove(event);
> 
> You should be able to call remove alone and not even do contains.

done

[David Grogan]

Created attachment 94003 [details]
Patch

[David Levin]

(In reply to comment #8)
> (From update of attachment 93497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93497&action=review
> > Is there a test that you know of that would have exposed this potential null pointer dereference?  I've just been running the layout tests.
> 
> Do we have existing types of tests that would catch object-lifetime problems?

That is a really general issue. So the answer is yes. But I think you mean do we have tests for EventQueue to detect lifetime issues with respect to ScriptExecutionContext, etc.

I don't know. I would guess not unless you write them.

But there are layout tests (which are just html files basically) that test various things. People around you should know about them if you need assistance.

[David Levin]

Comment on attachment 94003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94003&action=review

Just a few last comments.

> Source/WebCore/dom/EventQueue.cpp:86
> +        m_isCancelled = true;

You could release m_event here.

> Source/WebCore/dom/EventQueue.cpp:113
> +    m_eventTaskMap.add(event, task.get());

Ideally event.release() (since you don't need event anymore).

> Source/WebCore/dom/EventQueue.cpp:138
> +    if (task) {

Personally, I'd find

  if (!task)
     return false;
  task->cancel();
  removeEvent(event);
  return true;

to be more readable.

> Source/WebCore/dom/EventQueue.h:47
>  

If we get rid of the RefCounted nature of EventQueue, it makes it a lot clearer that there aren't lifetime issues.

I think the reason for making it refcounted (which you pointed out was https://bugs.webkit.org/show_bug.cgi?id=55512) is now gone.

> Source/WebCore/dom/EventQueue.h:49
> +    virtual ~EventQueue();

Why did this become virtual? (I don't see something new being derived from EventQueue.)

[David Grogan]

Comment on attachment 94003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94003&action=review

>> Source/WebCore/dom/EventQueue.cpp:86
>> +        m_isCancelled = true;
> 
> You could release m_event here.

Done.

>> Source/WebCore/dom/EventQueue.cpp:113
>> +    m_eventTaskMap.add(event, task.get());
> 
> Ideally event.release() (since you don't need event anymore).

I made this change, but what does it buy us?  Is the idea to avoid a ref + deref or is there more that I'm missing?

>> Source/WebCore/dom/EventQueue.cpp:138
>> +    if (task) {
> 
> Personally, I'd find
> 
>   if (!task)
>      return false;
>   task->cancel();
>   removeEvent(event);
>   return true;
> 
> to be more readable.

done

>> Source/WebCore/dom/EventQueue.h:47
>>  
> 
> If we get rid of the RefCounted nature of EventQueue, it makes it a lot clearer that there aren't lifetime issues.
> 
> I think the reason for making it refcounted (which you pointed out was https://bugs.webkit.org/show_bug.cgi?id=55512) is now gone.

Removed RefCounted, thanks for pointing this out.

>> Source/WebCore/dom/EventQueue.h:49
>> +    virtual ~EventQueue();
> 
> Why did this become virtual? (I don't see something new being derived from EventQueue.)

Because many years ago I was instructed to "always declare destructors as virtual" because you don't know if your class will ever be used as a base.

I suppose we don't because we don't want the extra runtime indirection?  Or we don't want a reader mistaking the virtual destructor for a signal indicating the existence of a derived class?

Reverted.

[David Grogan]

Created attachment 94036 [details]
Patch

[David Levin]

(In reply to comment #12)
> (From update of attachment 94003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94003&action=review

> >> Source/WebCore/dom/EventQueue.cpp:113
> >> +    m_eventTaskMap.add(event, task.get());
> > 
> > Ideally event.release() (since you don't need event anymore).
> 
> I made this change, but what does it buy us?  Is the idea to avoid a ref + deref or is there more that I'm missing?

That's all.  Just nice to do when you no longer need an object.

> >> Source/WebCore/dom/EventQueue.h:49
> >> +    virtual ~EventQueue();
> > 
> > Why did this become virtual? (I don't see something new being derived from EventQueue.)
> 
> Because many years ago I was instructed to "always declare destructors as virtual" because you don't know if your class will ever be used as a base.
> 
> I suppose we don't because we don't want the extra runtime indirection? 

Yes afaik. (It potentially slows things down.)

> Or we don't want a reader mistaking the virtual destructor for a signal indicating the existence of a derived class?

WebKit abhors virtuals. (Basically, you can use them but you must prove that you really need it.)

[WebKit Commit Bot]

Comment on attachment 94036 [details]
Patch

Clearing flags on attachment: 94036

Committed r86838: <http://trac.webkit.org/changeset/86838>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Feldman]

According to tonistiigi, this regressed inspector operation (https://bugs.webkit.org/show_bug.cgi?id=61653).

[David Grogan]

rolled back in bug 61653

[David Grogan]

Created attachment 109363 [details]
Patch

[David Grogan]

(Continuing from the comments in bug 61653)

This patch caused the script line numbers in web inspector to not scroll when the script is paused.

The problem is that pausing causes WTF::MainThread's functionQueue to no longer be serviced, preventing any tasks that were posted to the main thread from being dispatched, including the ScrollEvent's EventTask.  Scrolling worked before this patch because ActiveDOMObjects (including DOMTimers, which had been used) are only stopped on the page being inspected, not globally.

Dmitry described other effects:
"""
For example, suspending the script in a single page will stop all pages in the other tabs of the same single-process browser from doing Workers, Databases, FileAPI, media player and other things which use callOnMainThread().

If it was possible to suspend tasks per-ScriptExecutionContext, then it would be a better fix. The David's change would not regress things then because the main thread queue would be only suspended for the paused page. It would also avoid freezing various internal things that use callOnMainThread() for other, unrelated, pages.
"""

So... Yury or Pavel, could you comment on how feasible suspending tasks per-ScriptExecutionContext is and how much work it would be?


(I think this is the same problem causing the behavior described in bug 26801.)

[Pavel Feldman]

> So... Yury or Pavel, could you comment on how feasible suspending tasks per-ScriptExecutionContext is and how much work it would be?

I don't think you are asking the right people. But from what I know, suspending tasks on particular ScriptExecutionContext does not much sense since there may be synchronous interaction between them. What was motivation behind this change again? It sounds like it won't make things simpler anymore.

[David Grogan]

(In reply to comment #21)
> > So... Yury or Pavel, could you comment on how feasible suspending tasks per-ScriptExecutionContext is and how much work it would be?
> 
> I don't think you are asking the right people.

Oh, yikes, could you direct me to someone who would be more appropriate, if you know who that would be?

> But from what I know, suspending tasks on particular ScriptExecutionContext does not much sense since there may be synchronous interaction between them.   What was motivation behind this change again? It sounds like it won't make things simpler anymore.

Document currently maintains am event queue in addition to the main task queue.  Document uses this other queue for asynchronous DOM events.  (Async events are added to the queue and dispatched when a DOMTimer(0) fires.) Having two queues is problematic for reasons that I cannot express better than Dmitry Titov, so:

"Having 2 queues will sooner or later cause problems with ordering of tasks, termination, suspension and other things that all require some control on how queues operate."

So, that's the rationale.  I'm involved because I want to fire IndexedDB's asynchronous events from a worker so wanted to move the EventQueue from Document up into ScriptExecutionContext.

I don't consider myself well-enough armed to debate the merits of collapsing the two queues into one, but it seems odd that this only affects single-process safari.  Maybe I'm showing my ignorance, but If the correct behavior is to pause the global task queue, why isn't single-process chrome affected?  (This question isn't directed at Pavel necessarily, just to anyone who might know.)

Help and explanations are very welcome.

[Dmitry Titov]

(In reply to comment #21)
> > So... Yury or Pavel, could you comment on how feasible suspending tasks per-ScriptExecutionContext is and how much work it would be?
> 
> I don't think you are asking the right people. But from what I know, suspending tasks on particular ScriptExecutionContext does not much sense since there may be synchronous interaction between them. What was motivation behind this change again? It sounds like it won't make things simpler anymore.

Things seem to be in need of some fixing anyways :-) To rehash the issues, the 'pause' button in the debugger seems to freeze the queue used in callOnMainThread(). At the same time it tries to freeze SuspendableTimers, but only for the SWC under debugging. This already can cause the script in other pages to receive a JS callback on timer and synchronously operate on JS objects of the 'paused' page if they can cross-script each other. So it is already possible to modify the page under 'pause'.

At the same time, dev tools are implemented as HTML/JS page(s) as well - so they need some timers/events to be delivered to function properly. In case of in case of single-process environment, they need to be unfrozen and function in the same process with the debugged page. It randomly (?) turned out that David's patch that moved scroll events from timer-delivered to callOnMainThread-delivered broke dev tools pages because they don't update on scroll anymore when a script in the debugger is paused.

It doesn't seem the patch makes something more complicated then it already is. 

As a long term fix, I can see either disable having dev tools and debugged pages in the same process at all (and freeze all the pages in that process even better then today, with some visual cues as described in bug 26801), or we could become better in freezing only Tasks/Timers for debugged pages and continue to run others, while accepting that JS objects can be modified or even destroyed during 'pause' in the debugger from underneath us in case when pages cross-script each other, as is already possible today. Not sure how bad it is. 

I think that the question to dev tools folks was about this ^, how to debug a page when dev tools (also a page) are in the same process and use the same set of queues.

[David Grogan]

In the short term, to get IndexedDB to run on workers, can I change EventQueue to keep the current behavior for Document but post EventDispatcherTasks for WorkerContexts?

I posted a patch that does that a few months ago, I could just update it: https://bugs.webkit.org/attachment.cgi?id=90833&action=review

[Dmitry Titov]

(In reply to comment #24)
> In the short term, to get IndexedDB to run on workers, can I change EventQueue to keep the current behavior for Document but post EventDispatcherTasks for WorkerContexts?
> 
> I posted a patch that does that a few months ago, I could just update it: https://bugs.webkit.org/attachment.cgi?id=90833&action=review

This indeed sounds reasonable.

I think that' in light of complications that were uncovered by the attempt to do PostTask for each event instead of keeping a separate queue, your idea of keeping the implementation separate is a good one. While it's sad to see that a dual-queue solution grew into the code deep enough to become difficult to change, that investigation can happen separately from your work on IDB in Workers.

[David Grogan]

I'm not going to work on this anytime soon.

[Blaze Burg]

I am going to close this, as the entire discussion is moot now that Web Inspector is always out of process from the inspected page. And the original author has no intent of working on this further, and it's not needed for any other purpose AFAICT.

It would be nice to unify all work enqueued in a web content process's event loop, but I don't think anyone is advocating to do that with approach taken here (using ScriptExecutionContext::Task).