ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.89k stars 13.52k forks source link

bug: Failed to execute 'construct' on 'CustomElementConstructor' #20720

Closed BorntraegerMarc closed 4 years ago

BorntraegerMarc commented 4 years ago

Bug Report

Ionic version:

[x] 4.11.10

Current behavior:

I'm experiencing an exception when using local notifications in an ionic native application. The exception only happens sometimes and I was not able to come up with exact reproduction steps. I wanted to open this bug because maybe someone can help me to point to the right direction so I can come up with exact reproduction steps for this ionic bug.

Basically what happens is that the user receives a notification (while app is in foreground) and then he sends the app to background. After waiting for like 10-30mins he clicks on the notification (while the app is still in background; so not a cold start). By clicking on the notification we trigger a navigation command navigateForward which produces the following exception: Uncaught Error: Failed to execute 'construct' on 'CustomElementConstructor': The provided callback is no longer runnable.:

Screenshot 2020-03-06 at 09 37 22 Screenshot 2020-03-06 at 09 40 23

Expected behavior:

No exception is happening.

Steps to reproduce:

No clear reproduction steps available unfortunately. We were able to reproduce this bug mostly when app got sent to background for about 10-30mins & the phone screen got locked before clicking on the notification.

Related code:

In app.component:

async ngOnInit() {
    await this.platform.ready();
    // Omitted code to simplify example

    this.localNotifications.on('click').subscribe(async (not: ILocalNotification) => {
        // Omitted code to simplify example
        await this.zone.run(async () => {
            // ON NEXT LINE THE EXCEPTION HAPPENS
            await this.navCtrl.navigateForward([AppRoutes.CHAT + '/' + not.data.chat.type + '/' + not.data.chat.id], { animated: false }); 
        });
    });
}

Other information:

EDIT 1: I found this bug https://github.com/ionic-team/ionic/issues/17411 and tried to verify whether the bug is still happening with the mentioned fix. Unfourtunatly the bug was still reproducable in the following scenarios:

EDIT 2: Linking this issue to https://github.com/ionic-team/ionic/issues/20721 as they are related (both happens in the same scenario).

Ionic info:

Ionic:

   Ionic CLI                     : 6.2.0 (/Users/borntraegermarc/.nvm/versions/node/v12.13.0/lib/node_modules/@ionic/cli)
   Ionic Framework               : @ionic/angular 4.11.10
   @angular-devkit/build-angular : 0.803.23
   @angular-devkit/schematics    : 8.3.23
   @angular/cli                  : 8.3.23
   @ionic/angular-toolkit        : 2.1.2

Cordova:

   Cordova CLI       : 9.0.0 (cordova-lib@9.0.1)
   Cordova Platforms : android 8.1.0
   Cordova Plugins   : cordova-plugin-ionic-keyboard 2.2.0, cordova-plugin-ionic-webview 4.1.3, (and 24 other plugins)

Utility:

   cordova-res : not installed
   native-run  : 0.3.0

System:

   ios-sim : 8.0.2
   NodeJS  : v12.13.0 (/Users/borntraegermarc/.nvm/versions/node/v12.13.0/bin/node)
   npm     : 6.14.2
   OS      : macOS Catalina
   Xcode   : Xcode 10.3 Build version 10G8
liamdebeasi commented 4 years ago

Thanks for the issue. Can you provide a repo with the code required to reproduce this issue? I realize the issue states this only happens certain times, but without a code reproduction/steps to reproduce I am not sure I can be much help.

BorntraegerMarc commented 4 years ago

Thanks for your answer @liamdebeasi

Understood. So this confirms my suspicion that this is not a "straightforward" issue. Even for ionic experts :)

Please give us time until end of the week to come up with a simple repo with reproduction steps.

BorntraegerMarc commented 4 years ago

We were able to reproduce this in a small start project with exact reproduction steps ๐ŸŽ‰

Here's the code (make sure to checkout the exact commit): https://github.com/komed-health/ion-nav-bug-test/tree/0252637e7adb3d97f9ca0c09bda6b8226686f4c1

Which resulted in the following exception:

Screen Shot 2020-03-11 at 6 33 26 PM (1)

Also here is a video showing how we reproduced the exception: ezgif com-video-to-gif (1)

Only difference between video and "real life" reproduction is that the video was shortened: Instead of clicking on the notification immediately we left the app in background locked the phone for 2-3 hours. Then clicked on the notification

Is there anything more we can help you with identifying the bug?

BorntraegerMarc commented 4 years ago

The bug can be more easily reproduced after this commit: https://github.com/komed-health/ion-nav-bug-test/tree/c38c623e688a76f6cf75ec52e3fd9e4e888d4d37

Basically what you should do:

Attached couple of screenshots we took for debugging purposes. Maybe it helps: Archive.zip

BorntraegerMarc commented 4 years ago

Apparently this bug also reproduces on ionic5 & angular9...

BorntraegerMarc commented 4 years ago

@liamdebeasi were you able to reproduce the bug? :)

liamdebeasi commented 4 years ago

I was able to reproduce a crash; however, I was unable to reproduce the exact error you provided in the original post. For me, after tapping the notification on the screen the app just crashes for me and Android says "[App Name] has stopped working". Is this the same behavior you experienced? Or does the stay running?

BorntraegerMarc commented 4 years ago

Thanks for the answer, @liamdebeasi

Is this the same behavior you experienced?

It is not the same behavior as we experienced. For us the app stays running however we can't click anywhere to interact with it. It nearly seems that the new angular route gets loaded as an invisible "overlay" which blocks any other elements.

Because when you manually remove in the HTML DOM this "overlay" then you can interact with the app again.

Maybe you could share some more words on your testing steps or even share a video? Then I could say if you're doing something different...

liamdebeasi commented 4 years ago

Ahh the issue you reported is reproducible only in Cordova (I was using Capacitor). Ok let me dig into this some more now that I can reproduce the issue.

BorntraegerMarc commented 4 years ago

Cool, thanks @liamdebeasi

Is there anything we can help you with to debug/solve this bug?

duncan-c commented 4 years ago

I'm also seeing this exact bug in my app. The only difference is I'm using the FirebaseX plugin to receive FCM notifications (not the local-notifications plugin). In my case I'm using navigateForward to navigate to a tab, and it does navigate to the tab but doesn't load the tab content - it just shows a blank background.

BorntraegerMarc commented 4 years ago

@duncan-c yeah, I'd say it's the same bug. How a notification is generated and the "click" event is triggered probably doesn't make a big difference. Also whether navigate or navigateForward is used doesn't make a difference (it's basically the same).

liamdebeasi commented 4 years ago

@BorntraegerMarc My best guess right now is that the local notifications plugin is firing its click event before the cordova context has fully resumed. I tested this by having the click handler wait for the resume event to be fired before navigating, and the issue went away.

It sounds like either the plugin needs to wait or your app needs to wait for the resume event to be fired before proceeding. There must be something that the OS is doing to the webview when the app is in the background for longer than a certain period of time.

I came up with a workaround that you can use:

export class HomePage implements OnInit {  
  private readyApp!: () => void;
  private isAppInForeground: Promise<void> = Promise.resolve();

  constructor(
    private localNotifications: LocalNotifications,
    private readonly platform: Platform,
    private route: Router
  ) {}

  async ngOnInit() {
    await this.platform.ready();

    this.platform.pause.subscribe(() => {
      this.isAppInForeground = new Promise(resolve => { this.readyApp = resolve });
    });

    this.platform.resume.subscribe(() => {
      this.readyApp();
    })

    this.localNotifications.on("click").subscribe(async (data) => {
      await this.isAppInForeground;
      this.route.navigate(["/page1"]);
    });
  }
}

What this workaround does is lets you await the isAppInForeground variable to ensure that your app is ready for further navigation. It should also resolve if your app is never put into the background (for example if a user taps a notification banner that appears).

While the error seems to be coming from Ionic's codebase, I'm not sure it's a good idea to expect routing to work before the app has resumed fully, especially where the components are created asynchronously.

Can you give this workaround a try and let me know if it resolves the issue? I imagine this workaround can be used for https://github.com/ionic-team/ionic/issues/20721 as well.

BorntraegerMarc commented 4 years ago

Wow! I can only imagine how painful it must have been to debug & come up with this workaround ๐Ÿ˜„ Thanks a lot!

Can you give this workaround a try and let me know if it resolves the issue? I imagine this workaround can be used for #20721 as well.

Indeed it resolves both these issues. Even in our very complex production app!

There must be something that the OS is doing to the webview when the app is in the background for longer than a certain period of time.

Hmmm... Do you think we should maybe open a bug for the android webview? To verify that this behaviour is expected. Or would you say it's generally in the responsibilities of cordova / capacitor plugins to await the resume event?

liamdebeasi commented 4 years ago

This might just be an OS-level behavior that applies to all applications (I think iOS does something similar in terms of suspending apps in the background).

Regarding waiting for the resume event, I am not really sure whose responsibility that should be. I could see arguments being made for both the developer's application as well as the Cordova plugin.

I am going to close this issue as the main issue has been resolved. Thanks! ๐Ÿ˜„

BorntraegerMarc commented 4 years ago

This might just be an OS-level behavior that applies to all applications (I think iOS does something similar in terms of suspending apps in the background).

Right. I see your point that the app might be "stale" (not quite suspended / killed from system but still not active).

However there is still an exception happening in this "stale" scenario and I would like to get to the root to the problem.

There must be something that the OS is doing to the webview when the app is in the background for longer than a certain period of time.

So, would you say that there is an underlying bug in chrome that this exception Failed to execute 'construct' on 'CustomElementConstructor' is thrown or is it expected behaviour? Maybe it's not so easy to say ๐Ÿ™‚

I'm asking for your opinion @liamdebeasi to see whether I should create a bug report with chrome or not.

liamdebeasi commented 4 years ago

The errors we saw did indeed come from Ionic and Angular files, but I don't think the expectation should be that Ionic/Angular should work before the webview itself has been fully re-initialized/brought out of the background.

In terms of where to file the bug report, it depends on which part you think is incorrect. I think the main issue here is trying to run code before the resume event has fired. It could be that the local notification plugin author should wait for the resume event in which case a bug should be filed on their repo. That being said, I think this is more of a platform quirk than a bug. However, it might help to have more documentation on this.

ionitron-bot[bot] commented 4 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.