820971 - setting img.src=null triggers onerror when FirefoxOS app is run as inline activity

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

The Gaia Gallery app uses offscreen image tags for creating thumbnails of images.

After creating a thumbnail, the code sets the src property of the offscreen image to null to release memory.

When gallery runs as a regular app this works fine.

But when it runs as an inline activity (when used from the Contacts app to pick an image) setting src to null triggers the onerror handler.

Gecko shouldn't behave differently in these two cases.

https://bugzilla.mozilla.org/show_bug.cgi?id=819596 describes what I was seeing in Gallery.  I'm going to workaround this by setting the error handler to null before I set src to null, and with that patch will close bug 819596.

I'm filing this bug for the underlying Gecko issue.

Comment 1

[David Flanagan [:djf]]

Cc'ing Kyle because he knows about images and Fabrice because he knows about activities.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that per spec setting img.src = null will set it to the string "null", which I expect to generally fire onerror.  That's not the current behavior of XPConnect, since that stringifies null to "", but once we switch images to WebIDL bindings this will be the case.

What are you actually trying to accomplish by setting src to null, and would removeAttribute("src") do the trick?

Apart from that, this needs steps to reproduce...  The only way I can see setting src to "" firing onerror is if it's done before OnStopRequest for the imgIRequest, because then cancelling the request would show up in the load status. But it sounds like the src="" happens from onload already, right?

Comment 3

[David Flanagan [:djf]]

If I use img.removeAttribute('src') instead of img.src = null I don't see this bug anymore.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  What about if you use img.src="" ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Is there a difference in the value of img.complete in the two cases you're looking at?

Comment 6

[David Flanagan [:djf]]

Boris,

I was just trying the removeAttribute() workaround when you commented...

The steps to reproduce are in bug 819596, until I patch the Gallery app so it doesn't exhibit the bug anymore.

Yes, I'm setting src to null after the onload handler fires.

Could there be something about activities that makes null stringify differently?

Comment 7

[David Flanagan [:djf]]

Setting img.src = '' gives the same bug as using null

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  So it's not a stringification issue.

Does one of these cases have a base URI that does not match the document URI, perhaps?

Comment 9

[David Flanagan [:djf]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Is there a difference in the value of img.complete in the two cases you're
> looking at?

It is always true in the onload handler, when running as a regular app or as an activity.

It is false when the error handler is triggered in the activity case.

I'm not sure that was the test you had in mind, though.

Comment 10

[David Flanagan [:djf]]

document.URL is equal to document.baseURI when the onload handler triggers, in both cases.

In the app case, the URL is app://gallery.gaiamobile.org/index.html

And when launched as an activity the URL is: app://gallery.gaiamobile.org/index.html#pick

Could that #pick suffix make a difference?

Comment 11

[David Flanagan [:djf]]

I nominated this as blocking-basecamp because its Gaia symptom #819596 was blocking. I'll work around that symptom with removeAttribute('src') but it still seems like the underlying issue should be resolved.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, the #pick suffix is responsible here.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 691521 [details] [diff] [review]
Patch

Wrong diff.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm not sure that was the test you had in mind, though.

It was, yes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691522 [details] [diff] [review]
Patch

r=me.  I guess relative URI resolution drops the ref, eh?

Comment 18

[Andrew Overholt [:overholt]]

Not a blocker, but we'd like to see this fix uplifted.  Thanks.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 691522 [details] [diff] [review]
Patch

[Triage Comment]

This seems very safe and is the right thing to do.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f9c8a1c7e7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08f9c8a1c7e7

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c94da132e703
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bc778775048b