1027461 - Running Gaia Unit Tests in B2G Desktop Debug crashes browser on ImportManager assert

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Running Gaia Unit Tests in B2G Desktop built using debug causes the browser to crash on an assert:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/ImportManager.cpp#81

Possibly related to Bug 1016705

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

If we return an error code from OnStartRequest, we will still get a call to OnStopRequest (with our error being propagated). We need to handle that call gracefully.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Requesting blocking-2.0 because this will affect testing and is a regression.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8446835 [details] [diff] [review]
Proposed fix

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

::: content/base/src/ImportManager.cpp
@@ +269,5 @@
>                              nsresult aStatus)
>  {
> +  if (NS_FAILED(aStatus)) {
> +    // We failed in OnStartRequest, nothing more to do (we've already
> +    // dispatched an error event) just return here.

Does it always mean that we failed in OnStartRequest and not somewhere else in the process where we haven't dispatched error yet? I would check mStopped to just in case.

Thanks for fixing this by the way!

Comment 4

[Jason Smith [:jsmith]]

I don't think this is something we would block on if push came to shove, as we have no proof of user impact & this isn't causing downtime to a test suite that is already stood up in CI.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

That was a really good question. Here's a patch that addresses it.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8448364 [details] [diff] [review]
Patch v2

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

Yeah probably using a specific error is a good idea anyway.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 8

[Carsten Book [:Tomcat]]

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