Bug 30546 – Storage events don't quite match the current spec; need to fix

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Storage events should fire asynchronously.  Currently WebKit does so synchronously, but the spec seems fairly clear on the matter: http://dev.w3.org/html5/webstorage/#the-storage-event

[Jeremy Orlow]

By asynchronously, I mean via the event queue:

"""
When this happens, the user agent must queue a task to fire an event with the name storage, which does not bubble and is not cancelable, and which uses the StorageEvent interface, at each Window object whose Document object has a Storage object that is affected.
"""


Also note that we should not fire events for the current page:

"""
When the setItem(), removeItem(), and clear() methods are called on a Storage object x that is associated with a local storage area, if the methods did something, then in every HTMLDocument object whose Window object's localStorage attribute's Storage object is associated with the same storage area, other than x, a storage event must be fired, as described below.
"""

[Jeremy Orlow]

Not(In reply to comment #1)
> """
> When this happens, the user agent must queue a task to fire an event with the
> name storage, which does not bubble and is not cancelable, and which uses the
> StorageEvent interface, at each Window object whose Document object has a
> Storage object that is affected.
> """

Note also that the event is not supposed to bubble or be cancelable (which is not the current behavior).

[Jeremy Orlow]

Created attachment 46966 [details]
The WebCore part of this change

This change is going to require significant changes to every dom storage layout test that involves events.  It'd be great if someone could take a look at let me know if there's any major issues with it.  In the mean time, I'll be trying to find a clean way to transform all the layout tests.  At the moment, I'm leaning towards breaking the event layout tests into lots of little tests since otherwise it may be difficult to follow the very asynchronous nature of testing that'll be required.

[Darin Fisher (:fishd, Google)]

Do you need to worry about the Document being destroyed inside the call to dispatchWindowEvent?  It might be wise to swap m_storageEventQueue into a temporary variable so that once you begin calling dispatchWindowEvent you never need to access any members of the Document again.  (This is in general a good thing to do when calling out to foreign code.)

[Jeremy Orlow]

Also note that "if the event is being fired due to an invocation of the clear() method, the event must have its key, oldValue, and newValue attributes set to null."

[Jeremy Orlow]

Created attachment 47092 [details]
Patch

[Jeremy Orlow]

This meat of the patch I just posted is very simple.  It's just making events asynchronous, not posting them to the frame that generated them, passing a null for the key when issuing a clear storage event, and making the events non-cancelable/non-bubbleable...all of which are clearly stated in the spec.

The asynchronous and not posting to the frame that generated them forced me two re-write all the layout tests that dealt with storage events.  There's a lot of code there, but I tried to be fairly careful to ensure that test coverage did not shrink in any area.

[Jeremy Orlow]

Created attachment 47093 [details]
Oops...now with change log + all the files

[Jeremy Orlow]

Created attachment 47094 [details]
Also realized the descriptions for the tests were stale.  Fixed.

[Jeremy Orlow]

Comment on attachment 47094 [details]
Also realized the descriptions for the tests were stale.  Fixed.

Actually...still needs a bit of work.  Will post again once it's ready.

[Jeremy Orlow]

Created attachment 47242 [details]
Patch

[Jeremy Orlow]

Note that 90% of the patch is layout tests.  And at least 1/3 of that is deleting ones that have been replaced.  So while big, this patch isn't _that_ big.

[Darin Adler]

Comment on attachment 47242 [details]
Patch

> +void Document::storageEventTimerFired(Timer<Document>*)
> +{
> +    Vector<RefPtr<Event> > storageEventQueue;
> +    storageEventQueue.swap(m_storageEventQueue);
> +
> +    typedef Vector<RefPtr<Event> >::const_iterator Iterator;
> +    Iterator end = storageEventQueue.end();
> +    for (Iterator it = storageEventQueue.begin(); it != end; ++it)
> +        dispatchWindowEvent(*it);
> +}

Is there any time late in the lifetime of a document where the object has not been destroyed, but it would not be a good time to call dispatchWindowEvent? I guess the domWindow() function will return 0 in that case, so probably the answer is no.

r=me

[Jeremy Orlow]

(In reply to comment #13)
> (From update of attachment 47242 [details])
> > +void Document::storageEventTimerFired(Timer<Document>*)
> > +{
> > +    Vector<RefPtr<Event> > storageEventQueue;
> > +    storageEventQueue.swap(m_storageEventQueue);
> > +
> > +    typedef Vector<RefPtr<Event> >::const_iterator Iterator;
> > +    Iterator end = storageEventQueue.end();
> > +    for (Iterator it = storageEventQueue.begin(); it != end; ++it)
> > +        dispatchWindowEvent(*it);
> > +}
> 
> Is there any time late in the lifetime of a document where the object has not
> been destroyed, but it would not be a good time to call dispatchWindowEvent? I
> guess the domWindow() function will return 0 in that case, so probably the
> answer is no.

You probably know this stuff better than most people.  From what I can tell, the answer is no because the domWindow() returns 0.

Maybe I can make a layout test that tries this.  It could cause two storage events to fire and one of them can do a navigation of some sort.  But then what would the second do?  And what type of navigation would be the most likely to expose problems?

[Darin Adler]

(In reply to comment #14)
> Maybe I can make a layout test that tries this.  It could cause two storage
> events to fire and one of them can do a navigation of some sort.  But then what
> would the second do?  And what type of navigation would be the most likely to
> expose problems?

Your idea for a test does point out that the document might get deallocated by the first window event and then we could end up dereferencing a bad pointer in the second one. To make something bad happen the goal should be to have a storage event that has no references to the document so the document would actually get deallocated. Maybe three events:

    1) first event handler navigates
    2) second event handler triggers garbage collection
    3) crash before you get into the third event handler

The trick is to somehow construct an event handler for the third storage event that does not indirectly keep the document alive via, say, the DOM window. Except that probably the second event handler would never run because the first handler would make the document’s frame be 0.

I’m really not sure. But the object lifetimes here do seem a little unclear to me.

[Jeremy Orlow]

(In reply to comment #15)
> (In reply to comment #14)
> > Maybe I can make a layout test that tries this.  It could cause two storage
> > events to fire and one of them can do a navigation of some sort.  But then what
> > would the second do?  And what type of navigation would be the most likely to
> > expose problems?
> 
> Your idea for a test does point out that the document might get deallocated by
> the first window event and then we could end up dereferencing a bad pointer in
> the second one. To make something bad happen the goal should be to have a
> storage event that has no references to the document so the document would
> actually get deallocated. Maybe three events:
> 
>     1) first event handler navigates
>     2) second event handler triggers garbage collection
>     3) crash before you get into the third event handler
> 
> The trick is to somehow construct an event handler for the third storage event
> that does not indirectly keep the document alive via, say, the DOM window.
> Except that probably the second event handler would never run because the first
> handler would make the document’s frame be 0.
> 
> I’m really not sure. But the object lifetimes here do seem a little unclear to
> me.

What's the best (most deterministic) way to trigger a GC?

What objects will the storage event handler itself hold a reference to (and thus keep from being GCed)?

Is there anyone who is an expert on these object lifetimes that I can talk to?

[Darin Adler]

(In reply to comment #16)
> What's the best (most deterministic) way to trigger a GC?

DumpRenderTree has a GCController with a collect() function. See tests like fast/js/script-tests/cached-eval-gc.js for examples.

> What objects will the storage event handler itself hold a reference to (and thus keep from being GCed)?

Objects captured in its scope. It seems likely that will include the DOM window object since that's the global object.

> Is there anyone who is an expert on these object lifetimes that I can talk to?

Probably. (Sorry, smart ass answer; there is no real topic “object lifetimes”, but a lot of the experienced WebKit engineers know the things that affect the lifetimes of particular objects.)

[Jeremy Orlow]

Ccing a few others who might have ideas.

BACKGROUND: Storage events are now (as of this patch) queued up on the document.  We start a one shot timer that then processes the queue.  We're trying to figure out if there's any way that the document could go away while processing the queue.

The two cases that I think are most interesting and/or likely to expose issues would be to open a new window and have it listen to storage events.  The event handler could be something like this:

function storageEventHandler(e)
{
    if (e.newValue == "1")
        // navigate and/or close the window
    else if (e.newValue == "2")
        // force GC if in layout test controller
    else
        // do nothing?  or is there something more we can do?
}

The more that I think about this, though, it seems as though anything we might do would hold a reference to the Window.

I've tried playing around with this a bit in an ad-hoc way and haven't been able to produce any interesting results.  If anyone had any ideas or could point me towards a layout test that does something similar, it'd be great.  Otherwise I'm not sure if there's much left to be done.

[Darin Adler]

I did say review+ and r=me in the first place, which was intentional. If I had serious reservations I would not have done so.

[Jeremy Orlow]

Understood.  Sorry if I'm being neurotic.  I just figured this would be a good learning opportunity.

I have one Chromium patch that I'm hoping to land before this.  I expect I'll be able to land it Monday (whether or not it's with an additional layout test).

[Jeremy Orlow]

Committed r53840: <http://trac.webkit.org/changeset/53840>