"Assertion failure: !aTitle.IsEmpty()" with DOM Notification

RESOLVED FIXED in mozilla28

Status

()

--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jruderman, Assigned: baku)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla28
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe][qa-])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 822679 [details]
testcase

Assertion failure: !aTitle.IsEmpty(), at dom/src/notification/Notification.cpp:63
(Reporter)

Comment 1

5 years ago
Created attachment 822680 [details]
stack
Whiteboard: [systemsfe]
(Assignee)

Comment 2

5 years ago
Created attachment 823365 [details] [diff] [review]
notification.patch

Following the spec, title can be an empty string.
Attachment #823365 - Flags: review?(wchen)
(Assignee)

Updated

5 years ago
Assignee: nobody → amarchesini
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
Comment on attachment 823365 [details] [diff] [review]
notification.patch

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

Hijacking this review since the bug was caused by my code.

::: dom/tests/mochitest/notification/mochitest.ini
@@ +4,5 @@
>    NotificationTest.js
>  
>  [test_notification_basics.html]
>  [test_notification_storage.html]
> +[test_test_bug931307.html]

This should be:

[test_bug931307.html]

::: dom/tests/mochitest/notification/test_bug931307.html
@@ +13,5 @@
> +<script type="application/javascript"><!--
> +
> +new Notification("");
> +Notification.get();
> +ok(true, "No crash!");

The crash was happening in NotificationStorageCallback::Handle, which happens asynchronously from this content script. To be on the safe side, let's only succeed in the promise callback. So perhaps something like:

SimpleTest.waitForExplicitFinish();
new Notification("");
var promise = Notification.get();
promise.then(
  function onSuccess() {
    ok(true, "No crash!");
    SimpleTest.finish();
  },
  function onFailure() {
    ok(false, "Should not get an error in promise callback");
  }
);
(Assignee)

Comment 5

5 years ago
Created attachment 824387 [details] [diff] [review]
notification.patch
Attachment #823365 - Attachment is obsolete: true
Attachment #823365 - Flags: review?(wchen)
Attachment #824387 - Flags: review?(mhenretty)
Comment on attachment 824387 [details] [diff] [review]
notification.patch

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

LGTM!

We need some tests up in gaia for this too, so I'll add that to this bug.
Attachment #824387 - Flags: review?(mhenretty) → review+
Whiteboard: [systemsfe] → [systemsfe][keep open]
Created attachment 824739 [details] [review]
Pull Request for Gaia Integration Test

Once the Gecko patch lands, I'll re-kick off the travis test and flag Kyle for review.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Comment on attachment 824739 [details] [review]
Pull Request for Gaia Integration Test

Kyle, will you review this crash test up in gaia land?
Attachment #824739 - Flags: review?(kyle)
master: https://github.com/mozilla-b2g/gaia/commit/f79810d49bb83d515aacc4880468c0b29039c0cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Reporter)

Updated

5 years ago
Whiteboard: [systemsfe][keep open] → [systemsfe]

Updated

5 years ago
Whiteboard: [systemsfe] → [systemsfe][qa-]
You need to log in before you can comment on or make changes to this bug.