792471 - accessing properties in window.navigator crashes browser

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Christopher Van Wiemeersch [:cvan]]

http://people.mozilla.org/~cwiemeersch/nav.html crashes the Browser within gaia. I just get a nice frowny face.

Source code:

    for (var prop in navigator) {
        document.write('<dt>' + prop + '</dt><dd>' + navigator[prop] + '</dd>');
    }

Comment 1

[Andrew Overholt [:overholt]]

Andrea, can you take a look?

Comment 2

[Andrea Marchesini [:baku]]

yes. It crashes here: document.write(navigator.mozSms);
/me working on it...

Comment 3

[Andrea Marchesini [:baku]]

The reason why we have this crash using navigator.mozSms is:

1. if (!AppProcessHasPermission(this, "sms")) fails

2. it fails because in AppProcessHasPermission:
   TabParent* tab = static_cast<TabParent*>(aActor);
   nsCOMPtr<mozIApplication> app = tab->GetApp();
   here app is nullptr;

Probably we should not have navigator.mozSms if the current app/browser doesn't have the permissions for it. If this is what we want, I can fix it. I need a feedback.

Comment 4

[Mounir Lamouri (:mounir)]

It looks like we are doing a IPC call *before* checking the permission...

Here is what happens:
* Navigator::GetMozSms is called in the browser context
* Navigator::IsSmsSupported is called to check if the platform actually have a WebSMS backend (1)
* SmsManager::CheckPermissionAndCreateInstance (which is new to me) is a method that checks the permission then return an object if everything is fine (2)
* The object is returned (or null)

We likely crash at (1) because we have no permission in the browser app so when we do the IPC call to check for SMS support, we get killed by the main process (seems that this thing works \o/). We should clearly not do (1) before (2).

I think there are two solutions and both are pretty trivial:
1. Just move (1) after (2) but I don't really like it because that would require to nullify mSmsManager after setting it.
2. Given that all the logic already happens in SmsManager, we could just add the support check in that huge-method-that-I-don't-like-but-is-already-there-anyway. Doing that would be pretty trivial.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 663682 [details] [diff] [review]
patch

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

r=me

However, I'm not a big fan of the design. Mostly, having IsSmsSupported staying in Navigator sounds weird.

What do you think of this:
mSmsManager = GetInstanceIfAllowed(aWindow);
if (mSmsManager->IsSmsSupported()) {
  mSmsManager = nullptr;
}

That way, we prevent, by design, to have IsSmsSupported() being called without the permissions. Feel free to chose a better name for the method.

I know that design contradicts things I said above... :)

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch b

what about this second patch?

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 663699 [details] [diff] [review]
patch b

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

LGTM

::: dom/sms/src/SmsManager.cpp
@@ +89,5 @@
> +  NS_ENSURE_TRUE(smsService, nullptr);
> +
> +  bool result = false;
> +  smsService->HasSupport(&result);
> +  NS_ENSURE_TRUE(result, nullptr);

Don't use NS_ENSURE_TRUE here. If the |result| is false, it will warn but there is no reason to warn so you should have a simple if (!result) { return nullptr; }

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I don't see any Try results posted, so I've pushed this to be safe. I'll land it if the push is green.
https://tbpl.mozilla.org/?tree=Try&rev=7c6f0e0bae23

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch d

title updated

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch e

title + reviewer

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 664483 [details] [diff] [review]
patch d

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e3887667fa0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e3887667fa0