Closed Bug 1596825 Opened 11 months ago Closed 4 months ago

Intent not passed to Android Component

Categories

(GeckoView :: General, defect, P1)

72 Branch
Unspecified
All

Tracking

(firefox78 fixed)

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: royang51, Assigned: droeh)

References

Details

(Whiteboard: [geckoview:m78])

Attachments

(1 file)

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.

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

Rank: 8
Priority: -- → P2
Status: UNCONFIRMED → RESOLVED
Closed: 10 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1600704
Whiteboard: [geckoview:m1912]

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.

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---

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.

(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.

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?)

Assignee: nobody → droeh
Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

(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.

Flags: needinfo?(matt.woodrow)

(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.

Flags: needinfo?(matt.woodrow)

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)

Attachment #9147753 - Attachment description: Bug 1596825 - Call onLoadRequest for non-top-level load requests and add isTopLevel field to LoadRequest. r=snorp!,#geckoview-reviewers!,mattwoodrow! → Bug 1596825 - Add NavigationDelegate.onSubframeLoadRequest to handle non-top-level load requests. r=snorp!,#geckoview-reviewers!,mattwoodrow!

(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.

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!

Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/328927902c06
Add NavigationDelegate.onSubframeLoadRequest to handle non-top-level load requests. r=snorp,mattwoodrow,geckoview-reviewers,agi
Status: REOPENED → RESOLVED
Closed: 10 months ago4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Duplicate of this bug: 1640807
You need to log in before you can comment on or make changes to this bug.