Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error: null is not an object (evaluating 'new a.Image') #23935

Closed
dreamofabear opened this issue Aug 13, 2019 · 18 comments · Fixed by #26749 or #27204
Closed

Error: null is not an object (evaluating 'new a.Image') #23935

dreamofabear opened this issue Aug 13, 2019 · 18 comments · Fixed by #26749 or #27204
Labels
Type: Bug Type: Error Report An error reported by AMP Error Reporting WG: analytics

Comments

@dreamofabear
Copy link

Error: null is not an object (evaluating 'new a.Image')
at Image (https://raw.githubusercontent.com/ampproject/amphtml/1908121824440/src/window-interface.js:95)
at iframe (https://raw.githubusercontent.com/ampproject/amphtml/1908121824440/src/pixel.js:61)
at win (https://raw.githubusercontent.com/ampproject/amphtml/1908121824440/extensions/amp-analytics/0.1/transport.js:246)
at (https://raw.githubusercontent.com/ampproject/amphtml/1908121824440/extensions/amp-analytics/0.1/transport.js:132)
at requestUrl (https://raw.githubusercontent.com/ampproject/amphtml/1908121824440/extensions/amp-analytics/0.1/requests.js:260)

My guess is the window is destroyed by the time Transport.sendRequest is called. A null check may fix it.

/cc @ampproject/wg-analytics

@Enriqe
Copy link
Contributor

Enriqe commented Jan 28, 2020

@ampproject/wg-analytics @dvoytenko any updates here?

Still seeing 5.9k occurrences in the current 2001251659540 canary.

@dvoytenko
Copy link
Contributor

@zhouyx, is there some more magic we could do to bring this number down a bit more?

@dreamofabear
Copy link
Author

My guess is the window is destroyed by the time Transport.sendRequest is called. A null check may fix it.

Have we tried doing this?

@zhouyx
Copy link
Contributor

zhouyx commented Feb 11, 2020

No we haven't, shall i try checking the win before creating the pixel?

static sendRequestUsingImage(win, request, suppressWarnings, referrerPolicy) {
const image = createPixel(win, request.url, referrerPolicy);
loadPromise(image)

but I think the window is the value from this.win_ which is cached? Do you think it will help?

@dvoytenko
Copy link
Contributor

Hmm. It seemed like the stack traces were circling around Transport.sendRequest. But that API is not used for pixel, is it?

@dvoytenko
Copy link
Contributor

Oh. Just figured it out.

@dvoytenko
Copy link
Contributor

So, yeah, I think the problem is here:

const image = new Image();

So we just need to bailout there or somewhere before then when win == null

@zhouyx
Copy link
Contributor

zhouyx commented Feb 12, 2020

Ah so we pass in param by reference, makes sense. I'll create a quick fix.

@dvoytenko
Copy link
Contributor

The reason why it's hard to notice is probably b/c of the const Image = WindowInterface.getImage(win) in the:

const Image = WindowInterface.getImage(win);
const image = new Image();

It most likely gets inlined by the compiler, thus simply becoming:

const image = new win.Iimage();

@rcebulko
Copy link
Contributor

This is still occurring, and seems to occur only on iPhone. We have a new occurrence of this error appearing in Beta and Experimental exclusively on https://www-corriere-it.cdn.ampproject.org/ pages. It started last night 2020-03-11 some time 8pm-10pm, I'm guessing from a change they pushed to their site. It's currently reporting around 1000 errors an hour; that number will go up 10x if/when those releases are promoted.

@rcebulko rcebulko reopened this Mar 12, 2020
@rcebulko
Copy link
Contributor

By digging through the minified stacktrace, I was able to identify that this is happening via sendRequestUsingImage in transport.js

@rcebulko
Copy link
Contributor

It looks like the culprit may be here:

function createNoReferrerPixel(win, src) {
  if (isReferrerPolicySupported()) {
    return createImagePixel(win, src, true);
  } else {
    // if "referrerPolicy" is not supported, use iframe wrapper
    // to scrub the referrer.
    const iframe = createElementWithAttributes(
      /** @type {!Document} */ (win.document),
      'iframe',
      dict({
        'src': 'about:blank',
        'style': 'display:none',
      })
    );
    win.document.body.appendChild(iframe);
    createImagePixel(iframe.contentWindow, src);
    return iframe;
  }
}

All other callees of createPixel assert win. In this case, if the referrer policy isn't supported, it tries to create an image pixel in an iframe and then get the iframe's contentWindow. I suspect at this point in execution the iframe has not actually been added to the DOM and ready hasn't fired, so no contentWindow is available yet. Perhaps moving createImagePixel within an onReady handler would resolve this.

@ampproject/wg-analytics any thoughts on this?

@zhouyx
Copy link
Contributor

zhouyx commented Mar 12, 2020

Thank you @rcebulko for looking into this. I agree that the iframe contentWindow may not be ready yet.
I wonder what is causing the new occurrence though.

@rcebulko
Copy link
Contributor

@zhouyx Presumably some update to their site hits the exact corner case that triggers this bug

@zhouyx
Copy link
Contributor

zhouyx commented Mar 12, 2020

I found that they're using the vendor nielsen, which set referrerPolicy to no-referrer. Not sure if that's a recent change.
I also found that all errors are from Safari 13. That might be a browser issue, because referrerPolicy should have been supported. https://caniuse.com/#feat=mdn-html_elements_img_referrerpolicy
However I'm not able to reproduce this bug locally.

Thanks for the investigation. I'll fix the issue follow Ryan's suggestion. Since this is not a recent regression, I don't see a need for cherrypick.

@rcebulko
Copy link
Contributor

@zhouyx I already threw together a PR with a fix, if you'd prefer to take this over feel free to duplicate and close mine.

@rcebulko
Copy link
Contributor

I also can't produce locally because I don't have an iPhone (or a decent emulator of one). I agree it's not recent and probably doesn't merit a cherry-pick, though it will be one of the highest-volume errors being reported for a while.

@zhouyx
Copy link
Contributor

zhouyx commented Mar 12, 2020

Woohoo! Thank you! Left one comment.

Agreed that error number will remain high for a while. Can we mute this in the error log for two weeks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Type: Error Report An error reported by AMP Error Reporting WG: analytics
Projects
None yet
5 participants