Closed Bug 916893 Opened 11 years ago Closed 9 years ago

Worker - Support Notification API on workers.

Categories

(Core :: DOM: Workers, defect)

Product:
Core
Component:
DOM: Workers
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: nsm, Assigned: nsm)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 19 obsolete files)

MozReview Request: Bug 916893 - Notification on workers.
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 916893 - Deal with onclose. Some grammar fixes.
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 916893 - Walk up worker chain to find correct window for WorkerNotificationObserver.
39 bytes, text/x-review-board-request
khuey
: review+
Details
MozReview Request: Bug 916893 - Better ownership model
39 bytes, text/x-review-board-request
khuey
: review+
Details
Attached patch WIP patch (obsolete) — — Splinter Review
nsPrincipal::GetAppId() trips the assertion only on workers for some reason, but I'm not using an app. This is probably a bug in the list of checks done by requestPermission. Until NS_ProxyRelease has an equivalent on workers, this patch leaks the notification object from the various WorkerNotification* Runnables. BindToOwner() is never called in the worker case, not sure what problems this might cause. No Icon support off main thread yet. Depends on worker event loop patches.
Assignee: nobody → nsm.nikhil
(In reply to Nikhil Marathe [:nsm] from comment #1) > Until NS_ProxyRelease has an equivalent on workers, this patch leaks the > notification object from the various WorkerNotification* Runnables. I don't understand this, NS_ProxyRelease works everywhere.
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #2) Oh, I see, comments in bug 916203.
Depends on: 921221
No longer depends on: 914762
Kyle, I'd like feedback on the permission request code I've implemented - WorkerNotificationPermissionGetter, WorkerNotificationPermissionFeature and CallbackCaller. Am I using WorkerFeature properly and doing memory management that won't lead to crashes? If it is, then I'll port the event dispatch stuff to a similar style, that is hacky and leaky right now.
(In reply to Nikhil Marathe [:nsm] from comment #1) > Created attachment 805500 [details] [diff] [review] > WIP patch > > nsPrincipal::GetAppId() trips the assertion only on workers for some reason, > but I'm not using an app. This is probably a bug in the list of checks done > by requestPermission. > This bug no longer exists.
(In reply to Nikhil Marathe [:nsm] from comment #4) > Created attachment 810916 [details] [diff] [review] > Port Notification API to workers. (see notes in extended commit message) > > Kyle, I'd like feedback on the permission request code I've implemented - > WorkerNotificationPermissionGetter, WorkerNotificationPermissionFeature and > CallbackCaller. > Am I using WorkerFeature properly and doing memory management that won't > lead to crashes? > > If it is, then I'll port the event dispatch stuff to a similar style, that > is hacky and leaky right now. This intentionally does not popup a prompt since a worker may not always have relevant UI. We only do the main thread dance because the principal isn't useful on the worker thread.
Another question, Notification is modified to hold a rawptr to the worker so we can pass runnables to the worker. But the Notification is kept alive by the alert service observer as long as it is displayed on screen. If the owning window is closed and the worker gets CCed, and the notification is then removed from screen, the attempt to send the "close" event runnable to the worker will fail. Which option is better to keep the worker alive? 1) Hold nsRefPtr<> 2) Every Notification is a WorkerFeature.
Comment on attachment 810916 [details] [diff] [review] Port Notification API to workers. (see notes in extended commit message) Review of attachment 810916 [details] [diff] [review]: ----------------------------------------------------------------- There are some threading issues here I tried to point out. If you have specific questions about things please needinfo me. ::: dom/bindings/Bindings.conf @@ +824,5 @@ > 'resultNotAddRefed': [ 'item' ] > }, > > +'Notification': { > + 'wantsQI': False I don't think you need this anymore (I think it defaults to false for things that aren't nodes). ::: dom/src/notification/Notification.cpp @@ +17,5 @@ > #include "nsToolkitCompsCID.h" > #include "nsGlobalWindow.h" > #include "nsDOMJSUtils.h" > +#include "WorkerPrivate.h" > +#include "Events.h" I'm about to kill worker events. @@ +73,5 @@ > + bool > + Notify(JSContext* aCx, Status aStatus) > + { > + if (aStatus >= Canceling) > + mCanceled = true; brace your ifs @@ +90,5 @@ > + > +class CallbackCaller : public WorkerRunnable > +{ > +public: > + CallbackCaller(WorkerPrivate* aWorkerPrivate, WorkerNotificationPermissionFeature* aFeature, already_AddRefed<NotificationPermissionCallback> aCallback, NotificationPermission aPermission) break this across lines @@ +101,5 @@ > + } > + > + ~CallbackCaller() > + { > + mWorkerPrivate->RemoveFeature(mWorkerPrivate->GetJSContext(), mFeature); What guarantees that the destructor runs on the worker thread? @@ +103,5 @@ > + ~CallbackCaller() > + { > + mWorkerPrivate->RemoveFeature(mWorkerPrivate->GetJSContext(), mFeature); > + delete mFeature; > + mFeature = nullptr; Use an nsAutoPtr please. @@ +110,5 @@ > + bool > + WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) > + { > + if (mFeature->WasCanceled()) > + return true; brace your if @@ +112,5 @@ > + { > + if (mFeature->WasCanceled()) > + return true; > + > + MOZ_ASSERT(mCallback); Do this in the ctor. @@ +115,5 @@ > + > + MOZ_ASSERT(mCallback); > + ErrorResult rv; > + mCallback->Call(mPermission, rv); > + return !rv.Failed(); return true regardless, I think. @@ +138,5 @@ > + } > + > + ~WorkerNotificationPermissionGetter() > + { > + MOZ_ASSERT(NS_IsMainThread()); Again, what guarantees that your dtor runs on a specific thread? @@ +174,5 @@ > + } > + } > + > + nsRefPtr<CallbackCaller> c = new CallbackCaller(mWorkerPrivate, mFeature, mCallback.forget(), permission); > + return mWorkerPrivate->Dispatch(c) ? NS_OK : NS_ERROR_FAILURE; returning failure codes from nsIRunnable::Run doesn't really accomplish anything. @@ +576,5 @@ > > nsresult > Notification::ShowInternal() > { > + MOZ_ASSERT(NS_IsMainThread()); indentation. @@ +606,5 @@ > nsresult rv; > nsAutoString absoluteUrl; > if (mIconUrl.Length() > 0) { > // Resolve image URL against document base URI. > + MOZ_ASSERT(false); // FIXME for workers ... ::: dom/src/notification/Notification.h @@ +14,5 @@ > namespace dom { > > class NotificationObserver; > +namespace workers { > + class WorkerPrivate; no indentation. @@ +124,5 @@ > bool mIsClosed; > > static uint32_t sCount; > + > + workers::WorkerPrivate *mWorkerPrivate; // used only on Workers, is the GlobalObject's JSContext. I don't know what this comment means.
Attachment #810916 - Flags: feedback?(khuey) → feedback-
khuey, can you answer comment 7 please?
Flags: needinfo?(khuey)
Tested on B2G too. The "release Notification on worker thread" are the most niggly bits, though I'm pretty sure I've got all the code paths fixed now.
Attachment #820076 - Flags: review?(wchen)
Attachment #820076 - Flags: review?(khuey)
Comment on attachment 820076 [details] [diff] [review] Port Notification API to workers. Review of attachment 820076 [details] [diff] [review]: ----------------------------------------------------------------- If nothing else, this patch will run into the same shutdown problem that XHR had. If a WorkerRunnable that calls RemoveFeature is in the queue when the worker begins to shut down, it will get thrown away and RemoveFeature is never called, which leads to a shutdown hang. This is why we introduced RunWithClearing, which you'll need here for CallbackCaller. But it's not clear to me why we need two AddFeatures here. Isn't the Notification registered as a feature for its lifetime? If so, why do we need to have another feature for the permissions getter? ::: dom/src/notification/Notification.cpp @@ +36,5 @@ > +uint32_t Notification::sCount = 0; > + > +NotificationPermission > +GetPermissionPreCheck(nsIPrincipal* aPrincipal) > +{ Stick a MOZ_ASSERT(NS_IsMainThread()) here. @@ +100,5 @@ > +/** > + * Classes to drive Notification.requestPermission from Workers. > + * > + * Unlike main thread, requestPermission *WILL NOT* prompt the user for > + * permission on workers, it will use any existing permissions the origin has. Why not? @@ +126,5 @@ > + UnchangedBusyCount, SkipWhenClearing), > + mCallback(aCallback), > + mPermission(aPermission), > + mFeature(aFeature) > + { Either AssertIsOnMainThread or aWorkerPrivate->AssertIsOnParentThread as appropriate. @@ +136,5 @@ > + } > + > + bool > + WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) > + { aWorkerPrivate->AssertIsOnWorkerThread. @@ +145,5 @@ > + > + aWorkerPrivate->RemoveFeature(aCx, mFeature); > + > + // Release the reference to the callback. > + nsRefPtr<NotificationPermissionCallback> callback = mCallback.forget(); mCallback = null is sufficent. But I would actually prefer that you do this at the very top of the function, and then if (callback) etc... That way if someone adds an early return in the future they won't mess this up. ::: dom/src/notification/Notification.h @@ +9,5 @@ > > #include "nsDOMEventTargetHelper.h" > #include "nsIObserver.h" > > +#include "mozilla/dom/workers/bindings/WorkerFeature.h" We should just install this to mozilla/dom. File a bug? @@ +117,5 @@ > + > + static NotificationPermission GetPermissionInternal(nsIPrincipal* aPrincipal, > + ErrorResult& aRv); > + > + nsresult ResolveIconURL(nsAString& absoluteURL); aAbsoluteURL.
Attachment #820076 - Flags: review?(khuey) → review-
One other thing I forgot to comment on: WorkerSendEventAndReleaseRunnable is a bad idea. Dispatching events from a control runnable means they could get fired in the middle of random JS running (since control runnables preempt running JS). You should split this into two separate runnables, one that fires the event (that should just inherit from WorkerRunnable) and one that releases the notification (which should still be a control runnable). After you do that you should be able to drop the status check before firing the event too.
Depends on: 930159
No longer depends on: 916203
Attachment #820076 - Attachment is obsolete: true
Attachment #820076 - Flags: review?(wchen)
Kyle, I made the following changes: 1) CallbackCaller is RunWhenClearing. This means I'll have to check the worker status before calling the callback, correct? That is what I'm doing right now. 2) The WorkerSendEventAndReleaseRunnable is now WorkerRunnable, if it's dispatch fails, queue a WorkerControlRunnable to release the notification on the worker thread. This is done for "close" and "error". 3) For requestPermission(), I've filed a bug 930343. 4) Added a pref to disable notifications on workers by default since it's not in the Worker spec and we don't have a final decision for requestPermission(). [1]: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/041272.html
Comment on attachment 821423 [details] [diff] [review] Port Notification API to workers. Review of attachment 821423 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/notification/Notification.cpp @@ +34,5 @@ > namespace dom { > > +uint32_t Notification::sCount = 0; > + > +NotificationPermission static NotificationPermission @@ +121,5 @@ > + return true; > + } > +}; > + > +class CallbackCaller : public WorkerRunnable This is a very generic class name, it should probably be put into an unnamed namespace so that it doesn't conflict with anything outside of the translation unit. @@ +427,5 @@ > + bool > + WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) > + { > + // When release goes out of scope, Notification will be released. > + nsRefPtr<Notification> release = mNotification.forget(); mNotification = nullptr; @@ +493,5 @@ > + > + if (NS_FAILED(rv)) { > + // If the call failed, release the Notification object and remove the > + // Notification as a feature. > + r = new NotificationRemoveFeatureAndReleaseRunnable(mNotification.forget()); In NotificationTask::Run(), ownership of the |mNotification| may be transferred to a WorkerNotificationObserver object in |ShowInternal| and then return a failed return code (when GetAlertName fails). Then here we transfer ownership of the same notification again to NotificationRemoveFeatureAndReleaseRunnable which eventually leads to an extra release. @@ +567,5 @@ > +class WorkerSendEventRunnable : public WorkerRunnable > +{ > +public: > + WorkerSendEventRunnable(Notification* aNotification, > + const nsAString& aEventName) nit: indentation @@ +595,5 @@ > +public: > + WorkerSendEventAndReleaseRunnable(already_AddRefed<Notification> aNotification, > + const nsAString& aEventName) > + : WorkerRunnable(aNotification.get()->mWorkerPrivate, > + WorkerThread, UnchangedBusyCount, SkipWhenClearing), nit: indentation @@ +626,5 @@ > +NS_IMPL_ISUPPORTS1(WorkerNotificationObserver, nsIObserver) > + > +NS_IMETHODIMP > +WorkerNotificationObserver::Observe(nsISupports* aSubject, const char* aTopic, > + const PRUnichar* aData) nit: indentation @@ +756,5 @@ > + nsCOMPtr<nsIURI> srcUri; > + baseUri = doc->GetBaseURI(); > + NS_ENSURE_TRUE(baseUri, NS_ERROR_UNEXPECTED); > + nsresult rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(srcUri), > + mIconUrl, doc, baseUri); nit: indentation @@ +999,5 @@ > return NotificationPermission::Default; > } > } > > +class GetPrefRunnable : public nsRunnable This should also go into an unnamed namespace.
Attachment #821423 - Flags: review?(wchen) → review-
I'm going to wait on Bug 934731 to get fixed. Uploading the patch I have right now, which implements Navigator.get() and other storage stuff, but does not handle releasing things on the right threads when errors happen.
Attachment #821423 - Attachment is obsolete: true
Attachment #821423 - Flags: review?(khuey)
Latest patch. Works, no leaks. Needs some more refactoring and cleanup before review.
I'm going to wait for wchen to review this.
Attachment #8355029 - Attachment is obsolete: true
Attachment #8355029 - Flags: review?(wchen)
Attachment #8355029 - Flags: review?(khuey)
Comment on attachment 8358705 [details] [diff] [review] Port Notification API to workers. Review of attachment 8358705 [details] [diff] [review]: ----------------------------------------------------------------- I appreciate the effort of trying to simplify things a bit. ::: dom/src/notification/Notification.cpp @@ +141,5 @@ > + default: > + return NS_LITERAL_STRING("auto"); > + } > +} > +} } // anonymous namespace Also, newline before the brace, and after the matching opening brace. @@ +154,5 @@ > + MOZ_ASSERT_IF(!NS_IsMainThread(), !aWindow); > + > + JS::Rooted<JSObject*> global(aCx); > + if (NS_IsMainThread()) { > + nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aWindow); handle the case where sgo is null. @@ +163,5 @@ > + > + JSAutoCompartment ac(aCx, global); > + JS::Rooted<JSObject*> array(aCx, JS_NewArrayObject(aCx, 0, nullptr)); > + > + unsigned int count = 0; uint32_t @@ +216,2 @@ > > + NotificationStorageCallback(JSContext* aCx, nsPIDOMWindow* aWindow, Promise* aPromise) The JSContext isn't being used anymore here, also in Handle(). @@ +261,3 @@ > nsCOMPtr<nsPIDOMWindow> mWindow; > nsRefPtr<Promise> mPromise; > + std::vector<Notification::NotificationStrings> mNotifications; Any particular reason you aren't using an nsTArray here? and everywhere else you are using std::vector. @@ +283,5 @@ > NS_IMPL_CYCLE_COLLECTION_UNLINK_END > > +// Keeps a rawptr to a Notification because worker thread is being blocked so > +// the Notification won't go away. Should only be used from > +// PinnedNotification::Notify(). Let's enforce this by moving the constructor and destructor above |public:| and friending PinnedNotification. @@ +325,5 @@ > + * > + * When the worker thread state changes to Closing/Canceling/Killing, > + * Pinned::Notify() should be specialized for each type. > + */ > +template <class Ptr> This naming a little misleading because the template parameter expected here isn't actually a pointer type. @@ +367,5 @@ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + if (mPinned) { > + mWorkerPrivate->RemoveFeature(mWorkerPrivate->GetJSContext(), this); > + // Release. > + nsRefPtr<Ptr> n = mPtr.forget(); mPtr = nullptr; @@ +461,5 @@ > + mUnpinnable = nullptr; > + } > +}; > + > +template <class Unpinnable> Lets use something that looks more generic, like T. Unpinnable looks too much like a class name. @@ +644,5 @@ > +private: > + WorkerPrivate* mWorkerPrivate; > + nsAutoPtr<PinnedPromise> mPinnedPromise; > + std::vector<Notification::NotificationStrings> mNotifications; > + nsCOMPtr<nsISupports> mDummy; What? @@ +842,5 @@ > + return NS_OK; > + } > + > +private: > + JSContext* mCx; You don't need this. @@ +852,5 @@ > +class WorkerNotificationStorageGet : public nsRunnable > +{ > +public: > + WorkerNotificationStorageGet(WorkerPrivate* aWorkerPrivate, > + const GetNotificationOptions& aFilter, indentation @@ +891,5 @@ > + } > + > + // Pass ownership of pinned promise to callback. > + nsCOMPtr<nsINotificationStorageCallback> callback = new WorkerNotificationStorageCallback(mWorkerPrivate, pinnedPromise); > + autoUnpin.Clear(); How about renaming Clear() to forget() and having it return the pinned object so that it works more like a smart pointer. @@ +901,5 @@ > + return NS_OK; > + } > + > +private: > + WorkerPrivate* mWorkerPrivate; Pinned promise keeping this alive? If so, add a comment. @@ +1172,4 @@ > Notification::Notification(const nsAString& aID, const nsAString& aTitle, const nsAString& aBody, > NotificationDirection aDir, const nsAString& aLang, > const nsAString& aTag, const nsAString& aIconUrl, > nsPIDOMWindow* aWindow) Might as well fix the whitespace while you are here. @@ +1414,5 @@ > + > +private: > + nsRefPtr<Notification> mMainThreadNotification; > + WorkerPrivate* mWorkerPrivate; > + Notification* mWorkerThreadNotification; Add some comments about how the worker in conjunction with the pinned class are keeping these objects alive. Same thing everywhere else you do this. @@ +1621,5 @@ > + return mPermission; > + } > + > +private: > + WorkerPrivate* mWorkerPrivate; Needs comment that the object is kept alive on the stack. This class also needs comment mentioning that it can only be run synchronously. You should also add an assertion to make sure this object is only ever constructed from a worker thread. ::: dom/workers/RuntimeService.cpp @@ +147,2 @@ > #define PREF_WORKERS_LATEST_JS_VERSION "dom.workers.latestJSVersion" > +#define PREF_NOTIFICATIONS_ENABLED "dom.webnotifications.enabled" should this be dom.webnotifications.workers.enabled? likewise in the Workers.h comment.
Attachment #8358705 - Flags: review?(wchen)
(In reply to William Chen [:wchen] from comment #22) > > @@ +891,5 @@ > > + } > > + > > + // Pass ownership of pinned promise to callback. > > + nsCOMPtr<nsINotificationStorageCallback> callback = new WorkerNotificationStorageCallback(mWorkerPrivate, pinnedPromise); > > + autoUnpin.Clear(); > > How about renaming Clear() to forget() and having it return the pinned > object so that it works more like a smart pointer. I'm going to stick to Clear() since there is no well defined return value here for forget(). The AutoUnpin does not hold ownership, so its not a good idea to allow something else to reach the pointer. > > ::: dom/workers/RuntimeService.cpp > @@ +147,2 @@ > > #define PREF_WORKERS_LATEST_JS_VERSION "dom.workers.latestJSVersion" > > +#define PREF_NOTIFICATIONS_ENABLED "dom.webnotifications.enabled" > > should this be dom.webnotifications.workers.enabled? likewise in the > Workers.h comment. good catch!
Comment on attachment 8365451 [details] [diff] [review] Port Notification API to workers. Review of attachment 8365451 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/notification/Notification.cpp @@ +1287,5 @@ > + mWorkerPrivate, > + notification, > + NS_LITERAL_STRING("error")); > + > + // FIXME(nsm). You still have a FIXME here.
Fixed comment 25 and a lot of 80 character overflows.
Attachment #8366135 - Flags: review?(wchen)
Attachment #8365451 - Attachment is obsolete: true
Attachment #8365451 - Flags: review?(wchen)
Comment on attachment 8366135 [details] [diff] [review] Port Notification API to workers. Review of attachment 8366135 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/notification/Notification.cpp @@ +172,5 @@ > + > + uint32_t count = 0; > + for (uint32_t i = 0; i < aStrings.Length(); ++i) { > + NotificationOptions options; > + NotificationStrings st = aStrings[i]; Make this a const reference @@ +173,5 @@ > + uint32_t count = 0; > + for (uint32_t i = 0; i < aStrings.Length(); ++i) { > + NotificationOptions options; > + NotificationStrings st = aStrings[i]; > + options.mDir = StringToDirection(nsString(st.mDir)); st.mDir is already a nsString @@ +318,5 @@ > + { > + MOZ_COUNT_CTOR(Pinned); > + mWorkerPrivate->AssertIsOnWorkerThread(); > + mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(), this); > + mIsPinned = true; It looks like you could just test mPinnable everywhere you are using mIsPinned and remove the member. If for some reason you do need to keep it, get rid of this line and just set mIsPinned to true in the initialization list. @@ +344,5 @@ > + mWorkerPrivate->AssertIsOnWorkerThread(); > + if (mIsPinned) { > + mWorkerPrivate->RemoveFeature(mWorkerPrivate->GetJSContext(), this); > + // Release. > + nsRefPtr<T> n = mPinnable.forget(); You don't need this line. @@ +351,5 @@ > + } > + } > + > + bool > + Notify(JSContext* aCx, workers::Status aStatus) MOZ_OVERRIDE @@ +359,5 @@ > + } > +}; > + > +// Keeps a rawptr to a Notification because worker thread is being blocked so > +// the Notification won't go away. Should only be used from Can you move the part of the comment about the rawptr to just above the declaration of mNotification below? And perhaps mention that the worker thread is blocked because of running this runnable synchronously. @@ +370,5 @@ > + { > + MOZ_ASSERT(!NS_IsMainThread()); > + MOZ_ASSERT(mNotification); > +#ifdef DEBUG > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); If you can use DebugOnly<> for the WorkerPrivate, you won't need the #ifdef DEBUG macro. @@ +383,5 @@ > + } > + > +public: > + NS_IMETHODIMP > + Run() MOZ_OVERRIDE @@ +430,5 @@ > + MOZ_ASSERT(aStatus != Killing); > + > + if (aStatus >= Canceling) { > + MOZ_ASSERT(mIsPinned); > + nsRefPtr<Notification> kungFuDeathGrip = Get(); It doesn't look like you need this, the notification will be kept alive by the Pinned class in mPinnable until MaybeUnpin(). @@ +507,5 @@ > + AssertIsOnMainThread(); > + } > + > + bool > + WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) MOZ_OVERRIDE @@ +525,5 @@ > + T* mUnpinnable; > + bool mDelete; > + > +public: > + AsyncAutoUnpin(WorkerPrivate* aWorkerPrivate, T* aT, bool aDelete = false) At first glance, it looks like everywhere we use this class, aDelete is always passed in as true. If this is the case, then you can get rid of it and also simplify UnpinRunnable. @@ +724,5 @@ > + * Classes to drive Notification.get() and related operations from workers. > + */ > +class NotificationStoragePut : public nsRunnable > +{ > + class WorkerPrivateFeature : public WorkerFeature It's unfortunate that we can't use the Pinned template here. You need to add a comment about the purpose of this class. @@ +952,5 @@ > NotificationObserver(Notification* aNotification) > : mNotification(aNotification) {} > > + virtual ~NotificationObserver() { > + } nit: To stay consistent, this closing brace should go on the same line. @@ +1298,5 @@ > + > + Notification* notification = mWorkerPrivate ? mWorkerThreadNotification : > + mMainThreadNotification.get(); > + NotificationPermission perm; > + perm = Just put the equals on the previous line. @@ +1311,5 @@ > + nsRefPtr<WorkerSendEventRunnable> runnable = > + new WorkerSendEventRunnable( > + mWorkerPrivate, > + notification, > + NS_LITERAL_STRING("error")); What are we doing with the runnable? You should also add a test for this, make sure that we get the error event when we try to create a notification on a worker without permission. @@ +1527,3 @@ > > + nsAutoPtr<PinnedNotification> pinned( > + new PinnedNotification(workerPrivate, notification)); It looks like whenever we are creating a Pinned object, we are putting it in an nsAutoPtr and immediately forgetting it. You might as well just create them in the runnable so that it's less likely that we will forget to forget and end up with two autoptrs with a reference to the same object. @@ +1659,5 @@ > + if (aCallback.WasPassed()) { > + permissionCallback = &aCallback.Value(); > + } > + > + nsCOMPtr<nsIRunnable> request; Declare this when it's first used down below. @@ +1713,5 @@ > + } > + > +private: > + // When used from a SyncRunnable, the WorkerPrivate will stay alive because > + // it's thread is blocked and it is still in the Running state. nit: its
Attachment #8366135 - Flags: review?(wchen)
Made the following changes wrt attachment 8366135 [details] [diff] [review]: - Fixed issues from comment 27. - Added test for error fired on permission denied. - Moved permission to be a WorkerPrivate attribute so that run-to-completion semantics are followed on access. - Added test for child worker Notification creation. - Removed API related to Notification.get() until bugs in the main thread implementation are fixed in bug 965632 and bug 934731. - Worker Notification uses UTF8 to decode icon URL, since the worker's execution environment's URL encoding is always UTF8. - WorkerSendNotificationRunnable overrides Pre/PostDispatch to avoid parent thread assertions. It does not need ModifyBusyCount since the Worker is held alive by PinnedNotification. Confirming this with khuey via email.
Attachment #8369136 - Flags: review?(wchen)
Comment on attachment 8369136 [details] [diff] [review] Port Notification API to workers. Review of attachment 8369136 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/notification/Notification.cpp @@ +564,5 @@ > + */ > +class NotificationStoragePut : public nsRunnable > +{ > + // Dummy class that allows reuse of Pinned to keep the worker alive while > + // it's principal is accessed on the main thread. nit: its @@ +601,5 @@ > + { > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mWindow || mWorkerPrivate, "One of them should be non-null!"); > + > + // Don't ask! I think there needs to be a better comment here since it's not immediately obvious why this is needed. Perhaps mention that Run is called on the main thread and the pinned object must be unpinned in a worker thread. @@ +975,5 @@ > + NS_ENSURE_SUCCESS(rv, false); > + return true; > + } > +private: > + Notification* mNotification; What is keeping this notification alive? @@ +1102,5 @@ > + if (perm != NotificationPermission::Granted || !alertService) { > + // We do not have permission to show a notification or alert service > + // is not available. > + if (mWorkerPrivate) { > + //FIXME(nsm) test! ! ::: dom/workers/RuntimeService.cpp @@ +2377,5 @@ > + } > + > + WorkerDomainInfo* domainInfo = rts->mDomainMap.Get(host); > + if (!domainInfo) { > + //FIXME(nsm) ! @@ +2394,5 @@ > + > + uint32_t permManagerPermission; > + > + for (uint32_t index = 0; index < workers.Length(); index++) { > + permissionManager->TestPermissionFromPrincipal(workers[index]->GetPrincipal(), I'm not sure if workers use a null principal for anything (possibly a system principal), but TestPermissionFromPrincipal will fail if it gets one.
Attachment #8369136 - Flags: review?(wchen)
Mainly conflict fixes, but changes AsyncAutoUnpin dtor to thread specific releasing. Interdiff for other suggested fixes coming up.
Attachment #8378989 - Flags: review?(wchen)
Attached patch Interdiff for comment 29 (obsolete) — — Splinter Review
Modified WorkerSendEventRunnable (now WorkerNotificationSendEventRunnable) to take ownership of the Notification in case of permission denied error. See class comment for WorkerNotificationSendEventRunnable. Cleared up FIXME, with test_notification_permission.html for testing notification denied error (unfortunately this is in the full diff and not the interdiff) I can't think of a reason workers would have a null principal, but put in a check nonetheless.
Attachment #8378990 - Flags: review?(wchen)
Attachment #8378989 - Flags: review?(wchen) → review+
Attachment #8378990 - Flags: review?(wchen)
r+ for the notification bits.
Comment on attachment 8378989 [details] [diff] [review] Port Notification API to workers. Kyle, could you look at all the runnables once? The Unpin concept is borrowed from XHR.
Attachment #8378989 - Flags: review+ → review?(khuey)
I'm going to be a jerk here, but can you unbitrot these before I review? I promise I won't let them sit for a month this time :)
Flags: needinfo?(nsm.nikhil)
Rebased. interdiff has not changed.
Attachment #8398143 - Flags: review?(khuey)
Attachment #8378989 - Attachment is obsolete: true
Attachment #8378989 - Flags: review?(khuey)
Flags: needinfo?(nsm.nikhil)
Rebase again :( Interdiff remains unchanged.
Attachment #8402888 - Flags: review?(khuey)
Attachment #8398143 - Attachment is obsolete: true
Attachment #8398143 - Flags: review?(khuey)
Comment on attachment 8402888 [details] [diff] [review] Port Notification API to workers. Review of attachment 8402888 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't seem to incorporate wchen's review comments ... I'm not going to have time to look at this again, so you're going to have to get review from bent. ::: dom/src/notification/Notification.cpp @@ +230,2 @@ > { > + MOZ_COUNT_CTOR(NotificationStorageCallback); Don't add ctor/dtor macros to refcounted classes. @@ +256,5 @@ > } > > NS_IMETHOD Done(JSContext* aCx) > { > + // We've to resolve the promise in the compartment of the window. We have to ... The contraction does not apply in this case. @@ +266,1 @@ > return NS_OK; Break this method up with some newlines please.b @@ +305,5 @@ > + * When the worker thread state changes to Closing/Canceling/Killing, > + * Notify() will remove itself as a feature. > + * > + * Pinned<Notification> specializes Notify() to do some other stuff. > + */ I'm skeptical that this is generally useful. I think everything is going to end up specializing Notify to the point where it's not particularly useful. If the only thing we're accomplishing is holding a strong reference we can make AddFeature AddRef. @@ +355,5 @@ > + MOZ_ASSERT(mWorkerPrivate); > + mWorkerPrivate->AssertIsOnWorkerThread(); > + MOZ_ASSERT(mWorkerPrivate->GetJSContext() == aCx); > + > + MOZ_ASSERT(aStatus != Killing); Why do you think this assertion is valid? Workers can go straight to Killing, certainly at shutdown, possibly at other points. @@ +373,5 @@ > + CloseWorkerGoingAwayRunnable(Notification* aNotification) > + : mNotification(aNotification) > + { > + MOZ_ASSERT(mNotification); > + DebugOnly<WorkerPrivate*> workerPrivate = GetCurrentThreadWorkerPrivate(); We're still going to call GetCurrentThreadWorkerPrivate in an opt build here ... @@ +375,5 @@ > + { > + MOZ_ASSERT(mNotification); > + DebugOnly<WorkerPrivate*> workerPrivate = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); > + workerPrivate->AssertIsOnWorkerThread(); And this won't compile in an opt build. @@ +387,5 @@ > + > +public: > + NS_IMETHODIMP > + Run() MOZ_OVERRIDE > + { Needs a threading assertion. @@ +419,5 @@ > + nsRefPtr<CloseWorkerGoingAwayRunnable> r = > + new CloseWorkerGoingAwayRunnable(Get()); > + nsCOMPtr<nsIThread> mainThread; > + NS_GetMainThread(getter_AddRefs(mainThread)); > + SyncRunnable::DispatchToThread(mainThread, r); This means that the worker is blocked and unable to process control runnables here. I think we prefer the "async to the main thread, control runnable back" pattern we use elsewhere. @@ +429,5 @@ > + > + return true; > +} > + > +typedef Pinned<Promise> PinnedPromise; You don't actually use this, afaict. @@ +607,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mWindow || mWorkerPrivate, "One of them should be non-null!"); > + > + // Don't ask! > + Maybe<AsyncAutoUnpin<Pinned<Dummy>>> unpin; "Don't ask!" is not an acceptable comment for a patch you want to check into mozilla-central :P @@ +1062,5 @@ > + : mMainThreadNotification(aNotification), > + mWorkerPrivate(nullptr), > + mWorkerThreadNotification(nullptr) > + { > + MOZ_COUNT_CTOR(ShowNotificationRunnable); Again, no CTOR/DTOR in refcounted objects. @@ +1561,2 @@ > return nullptr; > } Why is this not implemented? ::: dom/workers/WorkerPrivate.cpp @@ +3050,5 @@ > + > + nsRefPtr<NotificationPermissionRunnable> runnable = > + new NotificationPermissionRunnable(ParentAsWorkerPrivate(), aPermission); > + if (!runnable->Dispatch(aCx)) { > + NS_WARNING("Failed to dispatch offline status change event!"); This is wrong.
Attachment #8402888 - Flags: review?(khuey) → review-
Attached patch Notification on workers (obsolete) — — Splinter Review
This patch is much cleaner than the abomination of a year ago. The worker feature part is inspired by WebSockets. Does not implement the Service Worker API - https://notifications.spec.whatwg.org/#service-worker-api Does not implement Notification.get() since the latest spec says it won't be available anywhere except ServiceWorkers anyway. William, I've refactored several things like resolving icon and sound urls, obtaining origins and persisting notifications into functions, please check that I'm calling them everywhere they previously used to be. Also do check that I'm sticking to spec. Ben Kelly, can you look at the part where I obtain a LoadContext from the worker? Kyle for everything else. Thanks!
Attachment #8540102 - Flags: review?(wchen)
Attachment #8540102 - Flags: review?(khuey)
Attachment #8540102 - Flags: feedback?(bkelly)
Comment on attachment 8540102 [details] [diff] [review] Notification on workers Review of attachment 8540102 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/notification/Notification.cpp @@ +1060,5 @@ > + nsCOMPtr<nsILoadGroup> loadGroup = mWorkerPrivate->GetLoadGroup(); > + nsCOMPtr<nsILoadContext> loadContext; > + NS_QueryNotificationCallbacks(nullptr, loadGroup, NS_GET_IID(nsILoadContext), > + getter_AddRefs(loadContext)); > + inPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing(); This looks reasonable to me. Note, however, that the private browsing flag is unconditionally set to false when creating a service worker or if we have to do NS_NewLoadGroup() in WorkerPrivate::GetLoadInfo(). I think we have a plan to fix this for SW by forbidding SW in private browsing mode. I'm fairly certain the other condition can only occur if the original document does not have a load group or if we are in chrome script without a window. Do those assumptions work for you?
Attachment #8540102 - Flags: feedback?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #39) > Comment on attachment 8540102 [details] [diff] [review] > Notification on workers > > Review of attachment 8540102 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/notification/Notification.cpp > @@ +1060,5 @@ > > + nsCOMPtr<nsILoadGroup> loadGroup = mWorkerPrivate->GetLoadGroup(); > > + nsCOMPtr<nsILoadContext> loadContext; > > + NS_QueryNotificationCallbacks(nullptr, loadGroup, NS_GET_IID(nsILoadContext), > > + getter_AddRefs(loadContext)); > > + inPrivateBrowsing = loadContext && loadContext->UsePrivateBrowsing(); > > This looks reasonable to me. > > Note, however, that the private browsing flag is unconditionally set to > false when creating a service worker or if we have to do NS_NewLoadGroup() > in WorkerPrivate::GetLoadInfo(). > > I think we have a plan to fix this for SW by forbidding SW in private > browsing mode. > > I'm fairly certain the other condition can only occur if the original > document does not have a load group or if we are in chrome script without a > window. > > Do those assumptions work for you? Yes, thanks!
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a06e5da24b6b and on b2g https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f88565852bf I'm not sure why there are flaky timeout warnings when I'm not using setTimeout, but I'll look into it later.
Comment on attachment 8540102 [details] [diff] [review] Notification on workers Review of attachment 8540102 [details] [diff] [review]: ----------------------------------------------------------------- The ownership model for the notification is very complex in this patch. It's pretty hard to see what owns the notifications at different points in the code. In some methods, the ownership is transferred to some other class half way through, and sometimes calling a method will take ownership and sometimes it doesn't (NotificationTask calling ShowInternal). If we really have to resort to all this manual reference counting everywhere for workers, it needs to be documented much better. Whenever calling a method that takes ownership of a reference, it should be commented (I've pointed out some cases in the review comments). Each of the raw notification pointers should have a comment saying that it owns the reference or say what is keeping it alive. I think we should also be more strict about transferring ownership of references. In particular, in NotificationTask::Run, once we call ShowInternal, that method should be responsible for taking ownership of the notification reference, either passing it on to the observer or releasing it instead of relying on the caller to check if it took ownership and release. The raw notification pointers that actually own the notification (such as in NotificationTask) should be commented as such and at very least we should use the convention of nulling it whenever the ownership has been transferred or the reference released and then assert that it's null in the destructor of the class. It may also instead make sense to replace the raw pointers with a helper class that does the check. ::: dom/notification/Notification.cpp @@ +494,1 @@ > mNotification->ShowInternal(); There should be a comment that ShowInternal may create an observer that takes ownership of the notification. @@ +621,2 @@ > // Queue a task to show the notification. > nsCOMPtr<nsIRunnable> showNotificationTask = There should be a comment that ownership of a notification reference is transferred to NotificationTask @@ +1455,5 @@ > + MainThreadRun() MOZ_OVERRIDE > + { > + // CloseNotificationRunnable is only called from Notify(), which is going > + // to handle releasing the reference, so make sure the observer's dtor > + // doesn't release it when the alertservice closes it. Is the observer the only thing that could be holding a reference to the notification when Notify gets called? It looks like another scenario might be that Notify gets called before NotificationTask::Run has the chance to run. Would that result in a double release (once from Notify and once from the NotificationTask::Run)? ::: dom/webidl/Notification.webidl @@ +22,3 @@ > static void requestPermission(optional NotificationPermissionCallback permissionCallback); > > + [Throws, Func="mozilla::dom::Notification::GetEnabled"] I find the GetEnabled slightly confusing here. Maybe call it IsGetEnabled?
Attachment #8540102 - Flags: review?(wchen) → review-
Comment on attachment 8540102 [details] [diff] [review] Notification on workers Dropping khuey review until I get back to wchen's suggestions.
Attachment #8540102 - Flags: review?(khuey)
Attached file MozReview Request: bz://916893/nsm (obsolete) —
/r/3795 - Bug 916893 - Notification on workers. /r/3797 - Bug 916893 - Better ownership model /r/3799 - Bug 916893 - Deal with onclose. Some grammar fixes. Pull down these commits: hg pull review -r 598cf57828d515d1b7cc4d15f8ea20269709c102
Attachment #8563681 - Flags: review?(wchen)
Improved ownership model with liberal use of UniquePtr. I can't think of a way to make it any nicer. Detailed comments in Notification.h, also addresses the possibility of double release when Notify() is called and no observer is around (short answer, it can't happen). Renamed to IsGetEnabled.
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm /r/3795 - Bug 916893 - Notification on workers. /r/3797 - Bug 916893 - Better ownership model /r/3799 - Bug 916893 - Deal with onclose. Some grammar fixes. Pull down these commits: hg pull review -r 598cf57828d515d1b7cc4d15f8ea20269709c102
https://reviewboard.mozilla.org/r/3797/#review4709 ::: dom/notification/Notification.cpp (Diff revision 1) > + // The NotificationRef is released by the task's dtor at the end of this I don't feel that comments like these are necessary when we have UniquePtr to manage the objects. ::: dom/notification/Notification.cpp (Diff revision 1) > // References are maintained by AddRefObject() and ReleaseObject(). Comment isn't necessary anymore, reference is maintained by NotificationRef. ::: dom/notification/Notification.cpp (Diff revision 1) > + mNotification->ReleaseObject(); Add an assertion to make sure we're on the right thread here? ::: dom/notification/Notification.cpp (Diff revision 1) > - NotificationTask(Notification* aNotification, NotificationAction aAction) > + NotificationTask(UniquePtr<NotificationRef> aRef, NotificationAction aAction) This should be a UniquePtr& ::: dom/notification/Notification.cpp (Diff revision 1) > - explicit NotificationObserver(Notification* aNotification) > + explicit NotificationObserver(UniquePtr<NotificationRef> aRef) This should be a UniquePtr& ::: dom/notification/Notification.cpp (Diff revision 1) > + if (mNotification) { Shouldn't mNotification always be non-null here? ::: dom/notification/Notification.h (Diff revision 1) > - NotificationFeature* mFeature; > + UniquePtr<NotificationFeature> mFeature; Is there a reason why this is a UniquePtr? It doesn't look like ownership is being passed around anywhere. ::: dom/notification/Notification.cpp (Diff revision 1) > - } > +Notification::ShowInternalRef(UniquePtr<NotificationRef> aRef) You could just do this in NotificationTask::Run and change ShowInternal to take a UniquePtr& instead of using mTempRef and the helper methods. ::: dom/notification/Notification.cpp (Diff revision 1) > + UniquePtr<NotificationRef> ownership; I don't think you need this, the callers should already be keeping the notification alive. ::: dom/notification/Notification.h (Diff revision 1) > + void CloseInternalNoRef(); I don't think we need CloseInternalNoRef, ClosedInternalRef, ShowInternalRef and mTemp. More details in comments below. ::: dom/notification/Notification.cpp (Diff revision 1) > + new ReleaseNotificationControlRunnable(mNotification); We seems to jump through some hoops to make sure that the notification's observer forgets its Notification reference in CloseNotificationRunnable. Perhaps it would make sense to steal the UniquePtr from the observer here and let ReleaseNotificationControlRunnable own it, then all it would have to do is clear out the pointer. This way you don't need the strange LeakNotification method and you don't need to deal with clearing out the observer in CloseNotificationRunnable. ::: dom/notification/Notification.cpp (Diff revision 1) > - if (!mNotification) { > + // runnables, see the Notification class comment. Instead of relying on the destructor of WorkerNotificationObserver to clear the notification reference. Can we instead make the worker runnables take the UniquePtr and have the runnables take care of the reference after it is done using it?
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm wchen has this covered for the moment
Attachment #8563681 - Flags: review?(khuey)
https://reviewboard.mozilla.org/r/3797/#review5097 > Is there a reason why this is a UniquePtr? It doesn't look like ownership is being passed around anywhere. I'm just using UniquePtr<> instead of nsAutoPtr<> > Shouldn't mNotification always be non-null here? Not if Leak() is called by the feature. > We seems to jump through some hoops to make sure that the notification's observer forgets its Notification reference in CloseNotificationRunnable. Perhaps it would make sense to steal the UniquePtr from the observer here and let ReleaseNotificationControlRunnable own it, then all it would have to do is clear out the pointer. > > This way you don't need the strange LeakNotification method and you don't need to deal with clearing out the observer in CloseNotificationRunnable. The problem is that Notification::mObserver should only be accessed on the main thread. Notify() runs on the worker thread and peeking into and modifying the observer's state there leads to the possibility that WorkerNotificationObserver::Observe tries to access a null notification. > I don't think we need CloseInternalNoRef, ClosedInternalRef, ShowInternalRef and mTemp. More details in comments below. I've removed the methods, but mTempRef is still needed unless {Show,Close}Internal() change to {Show,Close}Internal(UniquePtr<NotificationRef>& aTakeOwnership), which is a little harder to understand in my opinion. While ShowInternalRef() did take a param like that, it was static so conventionally having a 'object to operate on' was more readable there. > Instead of relying on the destructor of WorkerNotificationObserver to clear the notification reference. Can we instead make the worker runnables take the UniquePtr and have the runnables take care of the reference after it is done using it? The notification may receive multiple events. How would the event runnables pass ownership back to the observer after they were done running? > This should be a UniquePtr& That errors. The UniquePtr documentation also states "To unconditionally transfer ownership of a UniquePtr into a method, use a |UniquePtr| argument."
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm /r/3797 - Bug 916893 - Better ownership model Pull down this commit: hg pull review -r 7683d863aa085afae7a431f34ffcd1d462e46ae0
Attachment #8563681 - Flags: review?(khuey)
https://reviewboard.mozilla.org/r/3797/#review5225 > The problem is that Notification::mObserver should only be accessed on the main thread. Notify() runs on the worker thread and peeking into and modifying the observer's state there leads to the possibility that WorkerNotificationObserver::Observe tries to access a null notification. I've made one change here in the new diff. Directly called ReleaseObject(). Turns out ClearMainEventQueue does not WorkerRun() the queued runnables, but Cancel()s them, so we don't need to worry about event runnables touching a deleted notification.
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm /r/6209 - Bug 916893 - Notification on workers. /r/3797 - Bug 916893 - Better ownership model /r/6211 - Bug 916893 - Deal with onclose. Some grammar fixes. Pull down these commits: hg pull review -r df66614d828612b7d8c743b2b4e378969df92d3b
Attachment #8563681 - Flags: review?(khuey)
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm https://reviewboard.mozilla.org/r/3793/#review5227 Ship It!
Attachment #8563681 - Flags: review?(wchen) → review+
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm /r/6209 - Bug 916893 - Notification on workers. /r/3797 - Bug 916893 - Better ownership model /r/6211 - Bug 916893 - Deal with onclose. Some grammar fixes. Pull down these commits: hg pull review -r df66614d828612b7d8c743b2b4e378969df92d3b
Attachment #8563681 - Flags: review?(wchen)
Attachment #8563681 - Flags: review?(khuey)
Attachment #8563681 - Flags: review+
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm /r/6209 - Bug 916893 - Notification on workers. /r/3797 - Bug 916893 - Better ownership model /r/6211 - Bug 916893 - Deal with onclose. Some grammar fixes. Pull down these commits: hg pull review -r df66614d828612b7d8c743b2b4e378969df92d3b
Attachment #8563681 - Flags: review?(wchen) → review?(khuey)
Comment on attachment 8563681 [details] MozReview Request: bz://916893/nsm /r/6209 - Bug 916893 - Notification on workers. /r/3797 - Bug 916893 - Better ownership model /r/6211 - Bug 916893 - Deal with onclose. Some grammar fixes. /r/8247 - Bug 916893 - Walk up worker chain to find correct window for WorkerNotificationObserver. Pull down these commits: hg pull -r 61e34804edbb3462e58956cee00f0cdccfc01ac9 https://reviewboard-hg.mozilla.org/gecko/
Kyle, it would be really good to have Bug 1114554 shipping for 41 with Push. This is a blocker for that. Any ETA on the review?
Flags: needinfo?(khuey)
Attachment #8563681 - Attachment is obsolete: true
Attachment #8563681 - Flags: review?(khuey)
Attachment #8618037 - Flags: review?(khuey)
Attachment #8618038 - Flags: review?(khuey)
Attachment #8618039 - Flags: review?(khuey)
Attachment #8618040 - Flags: review?(khuey)
Attachment #8618041 - Flags: review?(khuey)
Attachment #8618042 - Flags: review?(khuey)
Comment on attachment 8618040 [details] MozReview Request: Bug 916893 - Notification on workers. Sorry, the conversion script that updated MozReview attachments to the new format didn't take into consideration discarded requests, because we didn't mean to allow people to discard individual commits (this is no longer possible in Review Board). I'm going to obsolete the attachments manually.
Attachment #8618040 - Attachment is obsolete: true
Attachment #8618040 - Flags: review?(khuey)
Attachment #8618042 - Attachment is obsolete: true
Attachment #8618042 - Flags: review?(khuey)
Comment on attachment 8618041 [details] MozReview Request: Bug 916893 - Better ownership model https://reviewboard.mozilla.org/r/3797/#review10437 Ship It! ::: dom/notification/Notification.h:65 (Diff revision 3) > + * UniquePtr<> is used throughout. Code should only access the underlying The first sentence of this is repeated above. ::: dom/notification/Notification.cpp:1606 (Diff revision 3) > - r->Dispatch(aCx); > + r->Dispatch(aCx); We should think more about whether this can be async. ::: dom/notification/Notification.h:90 (Diff revision 3) > - * isn't released as soon as ShowInternal() is done. Instead the observer > + * Notification object. We can end up in a situation where a event runnable is an event runnable ::: dom/notification/Notification.cpp:351 (Diff revision 3) > + Leak() Forget ::: dom/notification/Notification.cpp:859 (Diff revision 3) > + LeakNotification() Forget
Comment on attachment 8618039 [details] MozReview Request: Bug 916893 - Walk up worker chain to find correct window for WorkerNotificationObserver. https://reviewboard.mozilla.org/r/8247/#review10447 Ship It!
https://reviewboard.mozilla.org/r/3797/#review10453 > We should think more about whether this can be async. This cannot be async because the main thread runnable touches the notification, but since the feature has been notified, it will eventually remove itself so we can't guarantee the WorkerPrivate will stay alive. How about in Notify() dispatch runnable to main thread without blocking the worker. The CloseNotificationRunnable will then dispatch a control runnable back to the worker to remove the feature? The runnable has to run in the main thread since the observer has to be notified and CloseInternal() is main thread only.
Inbound's closed for orange caused by this. I'll backout in 10 min if a follow-up hasn't been pushed. 1976 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_serviceworker_interfaces.html | false: NotificationEvent should be defined on the global scope - expected PASS
Flags: needinfo?(nsm.nikhil)
Backed out for the aforementioned failures as well as Gaia orange. https://treeherder.mozilla.org/logviewer.html#?job_id=11109383&repo=mozilla-inbound
Flags: needinfo?(nsm.nikhil)
Depends on: 1196079
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: