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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrea Marchesini [:baku]]

Attachment: notification.patch

Following the spec, title can be an empty string.

Comment 3

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=4d2fdc68450d

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

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");
  }
);

Comment 5

[Andrea Marchesini [:baku]]

Attachment: notification.patch

Comment 6

[Michael Henretty [:mikehenrty][: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.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Pull Request for Gaia Integration Test

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4bd4c3aaa8db

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bd4c3aaa8db

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 824739 [details] [review]
Pull Request for Gaia Integration Test

Kyle, will you review this crash test up in gaia land?

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/f79810d49bb83d515aacc4880468c0b29039c0cd