1253199 - Assertion failure: globalObject, at dom/messagechannel/MessagePort.cpp:93

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

1. Install Spider
   http://bclary.com/projects/spider/spider-0.1.0.3-sm+tb+fx+an+fn.xpi

2. Load url in debug build using Spider
   firefox -spider -start -quit -url 'https://www.flickr.com/search/?text=portland'

3. Assertion failure: globalObject, at dom/messagechannel/MessagePort.cpp:93
   Nightly/47 Linux, Windows. OSX not tested.

Assertion added in bug 1250572 which is also pointed to by mozregression.

Note this is common in Bughunter (over 370 urls so far). Many of the urls are Google Maps urls. I didn't post them since they may contain PII.

I can't reproduce this without Spider so it may be an invalid bug caused by inappropriate usage in Spider itself. If so, if you can give me a hint on what Spider shouldn't be doing, I'd appreciate it.

Comment 1

[Andrea Marchesini [:baku]]

>    http://bclary.com/projects/spider/spider-0.1.0.3-sm+tb+fx+an+fn.xpi

I cannot download this file. 403.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: crash report Linux x86_64

This is from the minidump stackwalk output from Bughunter.

Comment 3

[Andrea Marchesini [:baku]]

Yes, I'm working on this. today I and bc spoke on IRC about how to reproduce this issue.

Comment 4

[Andrea Marchesini [:baku]]

I'm not able to reproduce it. Can you upload a full backtrace please?
I wonder if that happens on shutdown or if it's connected with CC/GC.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

From a quick code inspection, looks like the assertion may easily fire if MessagePort is used from an already closed Window.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: mp.patch

smaug, good catch!

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Could you explain the seemingly unrelated UpdateMustKeepAlive change?

And did you test this case? How do other browsers work when using MessagePort when their owner global has gone away? Though I'm mainly worried that did we end up changing behavior in bug 1250572?

Comment 8

[Andrea Marchesini [:baku]]

I don't see how we changed behavior. Also before, if the window goes away, globalObject is null and we end up showing a warning and exit.

About the other change, we call MessagePort::UpdateMustKeepAlive() only if the dispatching of the message succeeds. But maybe this is the last message that we want to dispatch, and in this case we should stop to keep alive the MessagePort object:

https://mxr.mozilla.org/mozilla-central/source/dom/messagechannel/MessagePort.cpp#914

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #8)
> I don't see how we changed behavior. Also before, if the window goes away,
> globalObject is null and we end up showing a warning and exit.
s/exit/return/ I think ;)
But you mean we had the same behavior before, right?


> About the other change, we call MessagePort::UpdateMustKeepAlive() only if
> the dispatching of the message succeeds. But maybe this is the last message
> that we want to dispatch, and in this case we should stop to keep alive the
> MessagePort object:
Ok, so is this a leak fix? Do we need that fix on branches. Leaks can cause horrible user experience.
Why we need that fix in this bug?

Comment 10

[Andrea Marchesini [:baku]]

> But you mean we had the same behavior before, right?

Yes, because previously the code was (at some point):

if (!globalObject || !jsapi.init(globalObject)) {
  return NS_ERROR_FAILURE;
}

> Why we need that fix in this bug?

I'll file a separate bug for this.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: mp.patch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

But could you test what other browsers do with MessagePorts coming from windows which have been deleted.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Since it is worrisome if the code path is actually executed rather often in the web pages.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85efbcc510b3

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85efbcc510b3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8728942 [details] [diff] [review]
mp.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1250572
[User impact if declined]: An assertion in debug builds
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Baku, Smaug, Overholt: I am bit concerned about this patch. We just removed the assert without any additional error handling. Is that a safe path to take? I am sure the assert was put into place for a reason.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

We removed a misplaced assertion.
(and assertions aren't error handling, they are just to assert something works as expected, but by default don't have any affect to release builds).

Comment 21

[Andrea Marchesini [:baku]]

Ritu, I added the assertion at the beginning, then I wrote the patch to remove it. Here why:

MessagePort has a completely custom life-time management. It can survive at its window (just for a little while) when dispatching messages (PostMessageRunnable keeps it alive). MessagePort::GetParentObject() is DOMEventTargetHelper::GetParentObject(). When the window is closing (and maybe also for other reasons) we call DOMEventTargetHelper::DisconnectFromOwner(), this nullify DOMEventTargetHelper::mParentObject. From that point GetParentObject() returns a nullptr.

If in the meantime we have a PostMessageRunnable runnable that is already be scheduled, when PostMessageRunnable::Run() is executed, it will have a nullptr from mPort->GetParentObject().

This is totally fine because the next line is:

    if (!globalObject || !jsapi.Init(globalObject)) {                                                            
      return NS_ERROR_FAILURE;
    }

and we stop the dispatching of that message.

The assert was there because I didn't consider this race condition when I wrote that code initially.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Andrea Marchesini (:baku) from comment #21)
> Ritu, I added the assertion at the beginning, then I wrote the patch to
> remove it. Here why:
> 
> MessagePort has a completely custom life-time management. It can survive at
> its window (just for a little while) when dispatching messages
> (PostMessageRunnable keeps it alive). MessagePort::GetParentObject() is
> DOMEventTargetHelper::GetParentObject(). When the window is closing (and
> maybe also for other reasons) we call
> DOMEventTargetHelper::DisconnectFromOwner(), this nullify
> DOMEventTargetHelper::mParentObject. From that point GetParentObject()
> returns a nullptr.
> 
> If in the meantime we have a PostMessageRunnable runnable that is already be
> scheduled, when PostMessageRunnable::Run() is executed, it will have a
> nullptr from mPort->GetParentObject().
> 
> This is totally fine because the next line is:
> 
>     if (!globalObject || !jsapi.Init(globalObject)) {                       
> 
>       return NS_ERROR_FAILURE;
>     }
> 
> and we stop the dispatching of that message.
> 
> The assert was there because I didn't consider this race condition when I
> wrote that code initially.

Thanks Baku for a detailed reply. It was mostly obvious but I still did not connect the dots. :( I review 20+ uplifts some days and blame it on fatigue. I appreciate a prompt reply, will approve it now.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728942 [details] [diff] [review]
mp.patch

Removing an assert that is not needed, Aurora47+

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/069a89a99547