1333981 - Label dom/storage actors and runnables

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This runnable seems a little weird to me. The runnable fires a generic observer to all windows. Then each nsGlobalWindow checks if the event applies to them (by checking the principals I think) and firing an event in that window. This is a problem for DocGroup labeling since a single runnable will run JS code for multiple windows. I think we want to split this into multiple runnables. Perhaps each window could use an AsyncEventDispatcher?

Honza, does that sound okay to you?

Comment 1

[Honza Bambas (:mayhemer)]

The mechanism can be a whatever way of notification.  

The purpose is to trigger the dom "storage event" notifying a change in a dom storage belonging to a window.  It has to arrive only at the window the modified storage belongs to, no where else.  The event is async.

The main handling is at [1].  Note the way we recognize which window accepts and which not the notification - [2] and [3].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#11584

[2] https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/dom/base/nsGlobalWindow.cpp#11625
[3] https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/dom/base/nsGlobalWindow.cpp#11655

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I see this runnable pretty often, so it's an important one. Any chance you could look into labeling it, Honza? There's information in the wiki here:
https://wiki.mozilla.org/Quantum/DOM
and I can answer any questions.

Comment 3

[Honza Bambas (:mayhemer)]

Jan, can you look at this please?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is this related to the DispatchLocalStorageChange message? That one also shows up a lot. Should that be a separate bug, or can it be fixed here too?

Comment 5

[Andrew Overholt [:overholt]]

Jan told me he'd work on this soon.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm going to repurpose this bug to cover all the labeling work needed in dom/storage. We can of course break this up more, but I don't have the knowledge to do that. The stuff I'm seeing is:

StorageNotifierRunnable
PStorage::Msg_LoadItem
PContent::Msg_DispatchLocalStorageChange
PStorage::Msg_LoadDone
PStorage::Msg_LoadUsage

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

There are several more runnables dispatching in dom/storage after searching the following keywords in dom/storage:
http://searchfox.org/mozilla-central/search?q=Dispatch%7CAsyncWait%7CTimer%7CAbstractThread%7CProxyRelease%7CReleaseOnMainThread%7CNewInputStreamPump%7CReleaseOnMainThread%7CnsMainThreadPtrHolder&case=false&regexp=true&path=dom%2Fstorage%2F

1. LoadUsageRunnable
   http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/storage/LocalStorageCache.cpp#651-652
2. dom::LocalStorageCacheBridge::Release
   http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/storage/LocalStorageCache.cpp#114-119
4. StorageObserver::mDBThreadStartDelayTimer
   http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/storage/StorageObserver.cpp#164

Comment 8

[Andrea Marchesini [:baku]]

jan, I didn't know you where working on this. Can I take it?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: storage.patch

Comment 10

[Andrea Marchesini [:baku]]

Attachment: storage.patch

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8883615 [details] [diff] [review]
storage.patch

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

Thanks, this is very nice. This should fix StorageNotifierRunnable. Unless you plan to fix them in this bug, could you please file bugs for the remaining messages in comment 6? I think we'll have to fix those using GetSpecificMessageEventTarget:
http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/dom/ipc/ContentChild.cpp#3544

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/326615f76fd9
Introduce StorageNotifierService for the dispatching of StorageEvent using the correct mainthread event target, r=billm

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for asserting in test_storageNotifications.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e831d66e4deb0b883eed0ed436eb9f232df3b4e6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=326615f76fd979750dfe844664070e1460b422e0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112242392&repo=mozilla-inbound
11:39:08     INFO -  6 INFO None7 INFO TEST-START | dom/tests/mochitest/storageevent/test_storageNotifications.html
11:39:08     INFO -  GECKO(3400) | ++DOMWINDOW == 24 (0AD43C00) [pid = 4060] [serial = 24] [outer = 0B6B4C00]
11:39:08     INFO -  GECKO(3400) | Assertion failure: !cx->enableAccessValidation || cx->compartment()->isAccessValid(), at z:/build/build/src/js/src/vm/Interpreter.cpp:372
...
11:55:02  WARNING -  TEST-UNEXPECTED-TIMEOUT | dom/tests/mochitest/storageevent/test_storageNotifications.html | application timed out after 330 seconds with no output

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/807fa40a522f
Introduce StorageNotifierService for the dispatching of StorageEvent using the correct mainthread event target, r=billm

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/807fa40a522f