1194087 - MailtoProtocolHandler, SmsProtocolHandler, and TelProtocolHandler will all fire twice after introduction of newChannel2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

See bug 1180273 comment 24 onwards for greater context, but basically:

- The protocol handlers b2g uses to convert uses of the mailto, sms, and tel protocols into web activities has always relied upon sending a childprocessmessagemanager message that results in an activity being triggered (AKA side-effect), then throwing an error to abort the channel opening since we don't actually want anything to happen.

- Bug 1147562 (or something) introduced newChannel2, but there was a missing "this." in the b2g stuff, which bug 1180273 fixed.  This is a problem because nsIOService::NewChannelFromURIWithProxyFlagsInternal will fall back to newChannel() from newChannel2() if the latter throws an error.  The b2g implementations of newChannel() just call newChannel2(), resulting in our double firing.

Potential fix-wise, in bug 1180273 comment 27, :sicking says:
> Would it be possible to move those into the AsyncOpen(2) functions instead?
> 
> Alternatively, it might work to have the newChannel function have the side
> effect, and let newChannel2 throw an exception. But I would prefer using
> AsyncOpen(2) instead.

I also went on a foray that suggested we could do something involving nsIWebBrowserChrome3 in bug 1180273 comment 28, but I figured it was too edgy.  I did validate that nsExternalProtocolHandler does go to the effort of creating a fake channel and hooking things up to AsyncOpen2 (in bug 1180273 comment 29), so that does seem like the best course of action.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

[Blocking Requested - why for this release]:
This problem leads to double-firing of activities.  This is mitigated when there are 2 or more apps that can service the activity, since there is system app support to ignore the second activity (with a test!).  But in the case where there is a single app to service the activity, the app will receive the two activity events in rapid succession.

Even in cases where the app takes efforts to compensate for double-fired activities (ex: email and handling double-single-clicks on mailto URLs prior to this bug), there can be emergent problems with the window manager, such as the app being backgrounded when it should not (bug 1192703 in email), or the system app/window manager state machine getting confused and failing to transition later on.  (A behaviour which led to email's behaviour that results in bug 1192703 manifesting.)

Comment 2

[Mahendra Potharaju [:mahe]]

Affecting other apps for 2.5. Blocking for 2.5

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: double-firing-handlers.patch

Andrew, I found a bug in the email app while testing that, but the gecko side looks sane now ;)

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8651252 [details] [diff] [review]
double-firing-handlers.patch

Works for me!  r=asuth (noting that I lack explicit review authority here and I'm assuming :fabrice has the authority to defer review to me here.)

Note that although the ActivityChannel implementation here is making an effort to completely stub nsIChannel, it is not stubbing nsIRequest which nsIChannel extends.  loadGroup/loadFlags seem most notable, but those look like they would be externally set via setter, so maybe XPConnect just clobbers the expandos into place?  Or it doesn't matter.  Since this is all a simple hack with an immediately-self closing channel, I'm going with it not mattering.

I'm also assuming that there's nothing meaningfully better we can do than throw exceptions (twice, because of the newChannel2 then newChannel cascade) when we're unable to parse out telephone numbers and sms numbers/bodies.  I suppose we could create a NoOpChannel, but that seems silly.

(In reply to [:fabrice] Fabrice Desré from comment #3)
> Andrew, I found a bug in the email app while testing that, but the gecko
> side looks sane now ;)

If the problem was a blank screen, then it's a bug in the platform custom elements implementation, bug 1176829.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> Comment on attachment 8651252 [details] [diff] [review]
> double-firing-handlers.patch
> 
> Works for me!  r=asuth (noting that I lack explicit review authority here
> and I'm assuming :fabrice has the authority to defer review to me here.)

I do!

> Note that although the ActivityChannel implementation here is making an
> effort to completely stub nsIChannel, it is not stubbing nsIRequest which
> nsIChannel extends.  loadGroup/loadFlags seem most notable, but those look
> like they would be externally set via setter, so maybe XPConnect just
> clobbers the expandos into place?  Or it doesn't matter.  Since this is all
> a simple hack with an immediately-self closing channel, I'm going with it
> not mattering.

yes, I initially added all the nsIRequest cruft, but that was useless. What mattered was setting the loadInfo & co. on the channel.

> I'm also assuming that there's nothing meaningfully better we can do than
> throw exceptions (twice, because of the newChannel2 then newChannel cascade)
> when we're unable to parse out telephone numbers and sms numbers/bodies.  I
> suppose we could create a NoOpChannel, but that seems silly.

Yes, that doesn't give us anything.

> (In reply to [:fabrice] Fabrice Desré from comment #3)
> > Andrew, I found a bug in the email app while testing that, but the gecko
> > side looks sane now ;)
> 
> If the problem was a blank screen, then it's a bug in the platform custom
> elements implementation, bug 1176829.

That's the one!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/1ccbe4224ba6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ccbe4224ba6