993435 - Crash getting 'navigator.connection' from closed window

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (requires popups) (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: manual testcase

Comment 2

[Jesse Ruderman]

Attachment: stack

Comment 3

[Jesse Ruderman]

bp-1ebe5cd6-3690-404e-a0d0-787532140408

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 960426.  In particular, this change:

    1.16 -  readonly attribute MozConnection? mozConnection;
    1.18 +  readonly attribute NetworkInformation connection;

but the actual getter can in fact return null, no?

     NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);

Comment 5

[John Shih]

(In reply to Boris Zbarsky [:bz] from comment #4)
> This is a regression from bug 960426.  In particular, this change:
> 
>     1.16 -  readonly attribute MozConnection? mozConnection;
>     1.18 +  readonly attribute NetworkInformation connection;
> 
> but the actual getter can in fact return null, no?
> 
>      NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);

It's a bug, I'll fix it.

Comment 6

[(no longer active)]

I think we should make this [Throws] and throw an exception in the error condition.

Comment 7

[John Shih]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> I think we should make this [Throws] and throw an exception in the error
> condition.

Yeah, I think we already do that. The bug is due to I didn't remove "NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);" as Boris pointed out when change the API to non-nullable.

Comment 8

[John Shih]

Attachment: Bug 993435 - Not return null for navigator.connection. r=bz

I've verified the crash isn't reproducible after applying the patch.

Comment 9

[John Shih]

Can you have a double check? Thanks very much!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8403728 [details] [diff] [review]
Bug 993435 - Not return null for navigator.connection. r=bz

r=me as long as the Connection object doesn't depend on there being a docshell.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc31b43ccafe

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc31b43ccafe

Comment 13

[Benjamin Kerensa [:bkerensa]]

Since this is not a regression and has been resolved not going to track this

Comment 15

[Jesse Ruderman]

LGTM