Closed Bug 1333981 Opened 3 years ago Closed 3 years ago

Label dom/storage actors and runnables


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




Tracking Status
firefox56 --- fixed


(Reporter: billm, Assigned: baku)




(1 file, 1 obsolete file)

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?
Flags: needinfo?(honzab.moz)
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].


Flags: needinfo?(honzab.moz)
Priority: -- → P2
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:
and I can answer any questions.
Flags: needinfo?(honzab.moz)
Jan, can you look at this please?
Flags: needinfo?(honzab.moz) → needinfo?(jvarga)
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?
Jan told me he'd work on this soon.
Assignee: nobody → jvarga
Flags: needinfo?(jvarga)
Priority: P2 → P1
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:

Summary: Label StorageNotifierRunnable → Label dom/storage actors and runnables
jan, I didn't know you where working on this. Can I take it?
Attached patch storage.patch (obsolete) — Splinter Review
Assignee: jvarga → amarchesini
Attachment #8883614 - Flags: review?(wmccloskey)
Attached patch storage.patchSplinter Review
Attachment #8883614 - Attachment is obsolete: true
Attachment #8883614 - Flags: review?(wmccloskey)
Attachment #8883615 - Flags: review?(wmccloskey)
Comment on attachment 8883615 [details] [diff] [review]

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:
Attachment #8883615 - Flags: review?(wmccloskey) → review+
Pushed by
Introduce StorageNotifierService for the dispatching of StorageEvent using the correct mainthread event target, r=billm
Blocks: 1378716
Backed out for asserting in test_storageNotifications.html:

Push with failures:
Failure log:
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
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Pushed by
Introduce StorageNotifierService for the dispatching of StorageEvent using the correct mainthread event target, r=billm
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1379568
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.