Closed Bug 1280106 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::PushSubscriptionChangeDispatcher::NotifyWorkers

Categories

(Core :: DOM: Push Notifications, defect, critical)

Unspecified
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: njn, Assigned: Lina)

References

Details

(Keywords: crash, Whiteboard: btpp-active)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-0e0dd898-2849-4746-b723-1083c2160604.
=============================================================

This crash first appeared in Nightly 20160602030220. Since then it has occurred 27 times. The crash address is always 0x0; it looks like |mPrincipal| is null.

kcambridge, any ideas? This looks like it was probably caused by bug 1275434.
Flags: needinfo?(kcambridge)
Bizarre! I'm wondering how a null gets in there, considering that the only caller uses a getter that should always return a principal. The fix is easy enough, but I'd like to understand how that happens.
Assignee: nobody → kcambridge
Blocks: 1275434
No longer blocks: 275434
Status: NEW → ASSIGNED
Flags: needinfo?(kcambridge)
Whiteboard: btpp-active
Attached patch nullCheckPrincipal.patch (obsolete) — Splinter Review
Attachment #8762763 - Flags: review?(dd.mozilla)
(In reply to Kit Cambridge [:kitcambridge] (PTO from 2016-06-20 to 2016-06-25) from comment #1)
> Bizarre! I'm wondering how a null gets in there, considering that the only
> caller uses a getter that should always return a principal. The fix is easy
> enough, but I'd like to understand how that happens.

Do you know why is this happening?
Flags: needinfo?(kcambridge)
(In reply to Dragana Damjanovic [:dragana] from comment #3)
> Do you know why is this happening?

Ha, I just figured this out. `dropRegistrationAndNotifyApp` calls `PushDB.delete()`, which returns a plain record instead of a push record, so `record.principal` isn't set.

I wrapped the null check in a warning, and added a test to make sure we're passing the principal to the observer.
Flags: needinfo?(kcambridge)
Attachment #8762763 - Attachment is obsolete: true
Attachment #8762763 - Flags: review?(dd.mozilla)
Attachment #8763315 - Flags: review?(dd.mozilla)
Attached patch useNotNull.patchSplinter Review
Nicholas, I see you added `NotNull` in bug 1272203 to mitigate this very problem. :-) Here's a WIP that converts PushNotifier to use it, if you think it'll help. I'm not sure if those explicit `.get()`s for casting can be cleaned up.
Attachment #8763316 - Flags: feedback?(n.nethercote)
Comment on attachment 8763316 [details] [diff] [review]
useNotNull.patch

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

It's a bummer that this conversion is a bit on the ugly side :(  And I have some questions below.

::: dom/ipc/ContentChild.cpp
@@ +3369,5 @@
>  {
>  #ifndef MOZ_SIMPLEPUSH
> +  auto principal = static_cast<nsIPrincipal*>(aPrincipal);
> +  PushMessageDispatcher dispatcher(aScope, WrapNotNull(principal), aMessageId,
> +                                   Nothing());

The need for this intermediate is unfortunate.

Actually, I don't understand how this compiles. How can a |const IPC::Principal&| be static_cast to an |nsIPrincipal*|? (This question applies to the old code as well, except no static_cast was involved.)

::: dom/push/PushNotifier.cpp
@@ +38,5 @@
>                                   nsIPrincipal* aPrincipal,
>                                   const nsAString& aMessageId,
>                                   uint32_t aDataLen, uint8_t* aData)
>  {
> +  NS_ENSURE_ARG(aPrincipal);

Even if you don't land this patch, I think this NS_ENSURE_ARG check (and the others below) are necessary, right? If so, perhaps they should be moved into the previous patch?

@@ +264,4 @@
>    // System subscriptions use observer notifications instead of service worker
>    // events. The `testing.notifyWorkers` pref disables worker events for
>    // non-system subscriptions.
> +  return !nsContentUtils::IsSystemPrincipal(mPrincipal.get()) &&

All the .get() calls are unfortunate. We can auto-convert nsCOMPtr<T> to T* and NotNull<T*> to T*, but NotNull<nsCOMPtr<T>> to T* requires a get().

::: dom/push/PushNotifier.h
@@ -78,5 @@
>  public:
>    PushNotifier();
>  
> -  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> -  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(PushNotifier, nsIPushNotifier)

I don't understand why these (and other) cycle collection macros are no longer needed. Can you explain?
Attachment #8763316 - Flags: feedback?(n.nethercote)
Comment on attachment 8763315 [details] [diff] [review]
nullCheckPrincipal.patch

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

::: dom/push/PushNotifier.cpp
@@ +276,5 @@
>  bool
>  PushDispatcher::ShouldNotifyWorkers()
>  {
> +  if (NS_WARN_IF(!mPrincipal)) {
> +    return false;

Do we still need this check?
Attachment #8763315 - Flags: review?(dd.mozilla) → review+
(In reply to Dragana Damjanovic [:dragana] from comment #8)
> Do we still need this check?

I think it'd be good to keep it, since it doesn't make sense for `ShouldNotifyWorkers` to return true if we don't have a principal.
(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #7)
> Actually, I don't understand how this compiles. How can a |const
> IPC::Principal&| be static_cast to an |nsIPrincipal*|? (This question
> applies to the old code as well, except no static_cast was involved.)

I'm guessing it's because of this overload? http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/ipc/PermissionMessageUtils.h#29 My C++-fu is weak, so it's likely I'm misreading that.

> Even if you don't land this patch, I think this NS_ENSURE_ARG check (and the
> others below) are necessary, right? If so, perhaps they should be moved into
> the previous patch?

Agreed; folded into previous patch.

> I don't understand why these (and other) cycle collection macros are no
> longer needed. Can you explain?

From bug 1233086, comment 20, it sounds like `nsIPrincipal` doesn't need to be cycle-collected, and `nsIPushMessage` doesn't hold any references to other CC participants.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c50f8365f8
Don't send push events to service workers without a principal. r=dragana
https://hg.mozilla.org/mozilla-central/rev/87c50f8365f8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8763315 [details] [diff] [review]
nullCheckPrincipal.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1275434.
[User impact if declined]: Content process crash when firing the `pushsubscriptionchange` event in some cases.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e1223a519bdf
[Risks and why]: Low risk; includes a test verifying the correct behavior.
[String/UUID change made/needed]: None.
Attachment #8763315 - Flags: approval-mozilla-aurora?
Comment on attachment 8763315 [details] [diff] [review]
nullCheckPrincipal.patch

Regression from 49, content process crash fix, includes tests. Uplift away!
Attachment #8763315 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.