1596825 - Intent not passed to Android Component

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[royang51]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Go to site:
https://teams.microsoft.com/dl/launcher/launcher.html?url=%2f_%23%2fl%2fhome%3ftenantId%12345%26lm%3ddeeplink%26lmsrc%3demail%26cmpid%3dFreemiumInviteEmail&type=home&deeplinkId=12345&directDl=true&msLaunch=true&enableMobilePage=true&suppressPrompt=true#

Click on the link "Open it" link.

Actual results:

No onLoadRequest() call to Android Component.

Expected results:

onLoadRequest() called so we can launch the deep link intent in Microsoft Team App.

Comment 1

[Emily Toop (:fluffyemily) On Leave until Jan 25th. Please NI [e-mail] instead.]

don't report load requests for top level. Needs investigation. Cross Origin request blocked.

Comment 3

[Dylan Roeh (:droeh) (he/him)]

Reopening because this is not fixed by the patch for bug 1600704 -- I can reproduce this, and it's busted differently than the cases from 1600704 and similar.

Comment 4

[Mattias]

Is there anything done about this? It is quite annoying as a user, since all links sent to Teams meetings have to be copied and opened in another browser.

Comment 5

[Dylan Roeh (:droeh) (he/him)]

(In reply to Mattias from comment #4)

Is there anything done about this? It is quite annoying as a user, since all links sent to Teams meetings have to be copied and opened in another browser.

Yes -- we had some major changes to make in how onLoadRequest works that initially seemed like they would fix this (bug 1619798), but didn't. The issue is that this tries to load the msteams: URI in an iframe, and we currently don't pass any non-top-level URI to onLoadRequest. We're exploring some possible API changes to let us handle app links loaded in iframes while still limiting onLoadRequest to top-level loads, so expect movement soon.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Matt, for one approach I'm considering for this it would be useful to load an error page if a promise resolves to a certain value (essentially, to have the content process hit nsDocShell:DisplayLoadError). I was hoping supplying the right error code (NS_ERROR_UNKNOWN_PROTOCOL) to DisconnectChildListeners or cancelling mChannel might work, but neither seem to. Can you think of any approach I could take here? (Is it even remotely reasonable?)

Comment 7

[Matt Woodrow (:mattwoodrow)]

NS_ERROR_UNKNOWN_PROTOCOL sort of intentionally avoids display an error page most of the time, see https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/docshell/base/nsDocShell.cpp#6241

Using DisconnectChildListener should hit that code there, which has the set of error code that result in an error page (different for top-level vs iframes!).

We can probably change some of that behaviour too if needed.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)

NS_ERROR_UNKNOWN_PROTOCOL sort of intentionally avoids display an error page most of the time, see https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/docshell/base/nsDocShell.cpp#6241

Using DisconnectChildListener should hit that code there, which has the set of error code that result in an error page (different for top-level vs iframes!).

We can probably change some of that behaviour too if needed.

Quick followup: Should I be passing NS_ERROR_UNKNOWN_PROTOCOL to both params of DisconnectChildListeners? I'm doing that currently and not hitting nsDocShell::DisplayLoadError at all.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

(In reply to Dylan Roeh (:droeh) (he/him) from comment #8)

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)

NS_ERROR_UNKNOWN_PROTOCOL sort of intentionally avoids display an error page most of the time, see https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/docshell/base/nsDocShell.cpp#6241

Using DisconnectChildListener should hit that code there, which has the set of error code that result in an error page (different for top-level vs iframes!).

We can probably change some of that behaviour too if needed.

Quick followup: Should I be passing NS_ERROR_UNKNOWN_PROTOCOL to both params of DisconnectChildListeners? I'm doing that currently and not hitting nsDocShell::DisplayLoadError at all.

Never mind, it was a problem with my code. Looks like we're hitting it correctly now.

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1596825 - Add NavigationDelegate.onSubframeLoadRequest to handle non-top-level load requests. r=snorp!,#geckoview-reviewers!,mattwoodrow!

Comment 11

[Ben Bucksch (:BenB)]

If you allow a little API feedback:

Load listeners in XUL work similarly in that they trigger for iframes. However, in the past, that has many times led to bugs in our code. We tested with a normal document, and it worked fine, and then much much later there was an iframe and it triggered the listener, and our code didn't expect that, and it broke in subtle but bad ways. This was despite the fact that this behavior was documented. We just didn't expect it and didn't think of it. It's just too surprising.

Now that we know of that "foot angle", of course we check for that. However, in almost all of our code, we always care about the top level load listeners only. So, we always have to add that check for the top level. That's a little annoying, because it clutters the code.

To avoid that, both to surprise new developers - and testers, because nobody thinks of testing this specific case -, and to avoid code clutter in the majority case, and avoid the "foot angle", here's a proposal:

Instead of modifying the existing listener, could you just add a new listener with a new name, specifically for this case? And the existing one keeps its current behavior?

Because it my experience, you almost always care only about the top level. Be it URL bar or security indicators, or progress listeners, or a load finished listener. Very rarely do we care about iframes.

Discussion on mobile-firefox-dev mailling list (direct link to the post)

Comment 12

[Dylan Roeh (:droeh) (he/him)]

(In reply to Ben Bucksch (:BenB) from comment #11)

If you allow a little API feedback:

Load listeners in XUL work similarly in that they trigger for iframes. However, in the past, that has many times led to bugs in our code. We tested with a normal document, and it worked fine, and then much much later there was an iframe and it triggered the listener, and our code didn't expect that, and it broke in subtle but bad ways. This was despite the fact that this behavior was documented. We just didn't expect it and didn't think of it. It's just too surprising.

Now that we know of that "foot angle", of course we check for that. However, in almost all of our code, we always care about the top level load listeners only. So, we always have to add that check for the top level. That's a little annoying, because it clutters the code.

To avoid that, both to surprise new developers - and testers, because nobody thinks of testing this specific case -, and to avoid code clutter in the majority case, and avoid the "foot angle", here's a proposal:

Instead of modifying the existing listener, could you just add a new listener with a new name, specifically for this case? And the existing one keeps its current behavior?

Because it my experience, you almost always care only about the top level. Be it URL bar or security indicators, or progress listeners, or a load finished listener. Very rarely do we care about iframes.

Discussion on mobile-firefox-dev mailling list (direct link to the post)

Thanks! This was really good feedback, and after discussing internally we decided to go with preserving onLoadRequest as only being for top-level requests and adding onSubframeLoadRequest to satisfy the requirements we have here. I'll be sending out a more detailed email to mobile-firefox-dev before the patch lands.

Comment 13

[Ben Bucksch (:BenB)]

Hey Dylan, thanks so much, for reaching out to the community with your posting, and also for listening to the feedback! I'm glad you liked my suggestion. I'm grateful. I wish you well!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/328927902c06
Add NavigationDelegate.onSubframeLoadRequest to handle non-top-level load requests. r=snorp,mattwoodrow,geckoview-reviewers,agi

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/328927902c06