Closed Bug 792471 Opened 7 years ago Closed 7 years ago

accessing properties in window.navigator crashes browser

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: cvan, Assigned: baku)

References

()

Details

(Keywords: crash, testcase)

Attachments

(1 file, 4 obsolete files)

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>');
    }
blocking-basecamp: --- → ?
Keywords: crash, testcase
Andrea, can you take a look?
Assignee: nobody → amarchesini
blocking-basecamp: ? → +
yes. It crashes here: document.write(navigator.mozSms);
/me working on it...
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.
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.
Component: General → DOM
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Component: DOM → DOM: Device Interfaces
Attached patch patch (obsolete) — Splinter Review
Attachment #663682 - Flags: review?(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... :)
Attachment #663682 - Flags: review?(mounir) → review+
Attached patch patch b (obsolete) — Splinter Review
what about this second patch?
Attachment #663682 - Attachment is obsolete: true
Attachment #663699 - Flags: review?(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; }
Attachment #663699 - Flags: review?(mounir) → review+
Attached patch patch c (obsolete) — Splinter Review
Attachment #663699 - Attachment is obsolete: true
Attachment #664023 - Flags: review+
Keywords: checkin-needed
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
Attached patch patch d (obsolete) — Splinter Review
title updated
Attachment #664023 - Attachment is obsolete: true
Attachment #664483 - Flags: review+
Attached patch patch eSplinter Review
title + reviewer
Attachment #664483 - Attachment is obsolete: true
Attachment #664485 - Flags: review+
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla18
Keywords: verifyme
QA Contact: jsmith
https://hg.mozilla.org/mozilla-central/rev/0e3887667fa0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.