1045096 - window.performance throws NS_ERROR_FAILURE in iframes created with a javascript: URI if touched before the javascript: returns a string

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Philip Tellis]

Attachment: test-firefox-bug.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.35 Safari/537.36

Steps to reproduce:

UserAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0"

See attached html file.

Create an anonymous iframe and write html into it using document.write.  After the iframe's onload event fires, try to access window.performance inside the iframe.

I have narrowed down the problem to the nightly on 2014-06-27 (31 b5).  The nightly on 2014-06-26 does not have this problem.

This problem still exists in 32.


Actual results:

Exception thrown: NS_ERROR_FAILURE, with no error message:

Exception { message: "", result: 2147500037, name: "NS_ERROR_FAILURE", filename: "file:///Users/philip/test-firefox-bug.html", lineNumber: 24, columnNumber: 0, inner: null, data: null }



Expected results:

No exception should be thrown, and actual window.performance object should be returned (this is how it works on Chrome, IE & Opera)

Comment 1

[Philip Tellis]

Note the error shows up even if the page is served over HTTP.

Comment 2

[Robin Whittleton]

Looks like the same error on the new Airbnb (e.g. https://www.airbnb.co.uk/s/reykjavik ) so this is in the wild.

Comment 3

[Philip Tellis]

yes, we're seeing this across thousands of websites that use boomerang

Comment 4

[Alvin Sng]

I can confirm that this is happening over at Airbnb, we will remove the window.performance calls  from Firefox on Airbnb.com if this is not fixed soon.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I can see the issue in the original testcase on this bug, but not at https://www.airbnb.co.uk/s/reykjavik

Alvin, has airbnb already changed something on their end?  It would be good to verify the fix for the original testcase here against whatever issue you were seeing...

Comment 6

[Philip Tellis]

@Alvin, instead of removing the code, I've found that wrapping it in a try/catch is sufficient to suppress the error on Firefox.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So on the original testcase in this bug, doing window.performance in Firefox 30 returned null.  In Firefox 31 it throws, due to the fix for bug 1025078.

Philip, did the actual code you reduced the testcase from not actually try to _do_ anything with the performance object?  It just got it but then didn't touch it?

In any case, the document.open()/write()/close() bit is a red herring: it's not supposed to change anything about the performance object.  I'll attach a minimal testcase for the actual issue.

[Blocking Requested - why for this release]:
[Tracking Requested - why for this release]:
[Tracking Requested - why for this release]:
Because it can break web pages.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: More minimal testcase

Comment 10

[Boris Zbarsky [:bzbarsky]]

So the issue is that we explicitly don't MaybeInitTiming() for a javascript: load in nsDocShell::InternalLoad, as part of a fix for bug 748276.

Olli, how would you feel about just smacking a timing object on the document in CreateAboutBlankContentViewer, to make sure the initial about:blank always has a timing object?

Comment 11

[Philip Tellis]

The original code is here: https://github.com/bluesmoon/boomerang/blob/7b2dab543b046418d0506184af4898623bad5692/plugins/rt.js#L205-L223

It looks through window.performance.getEntriesByName for resources that match a URL (ResourceTiming API), however the failure happens before it gets that far.  Note that Anonymous iframes can still contain resources.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, so that code is a no-op if window.performance returns null.  That explains why it was "working" before.

Comment 13

[Philip Tellis]

yes.  doesn't everyone do capability checks in JavaScript before using a capability ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure initial about:blank in iframes have a nsDOMNavigationTiming object, so we don't end up with window.performance throwing when accessed on their window

Comment 15

[Boris Zbarsky [:bzbarsky]]

> doesn't everyone do capability checks in JavaScript before using a capability

A number of people do the checks using "in", because boolean testing might not do what you want in many cases.  ;)

Comment 16

[Philip Tellis]

duly noted, but I still need to do the boolean test to avoid a null dereference.

Comment 17

[Alvin Sng]

Hey sorry for coming in late.

I see that Boris has created a fix on the Firefox end and that's great! We have not fixed our end and is working on putting the code in a try/catch block.

We experience the exception some of the time on Mac running FF31 and FF32 but it occurs all of the time with FF31 on linux.

Hope that information helps.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> duly noted, but I still need to do the boolean test to avoid a null dereference.

Yeah, due to browser bugs.  :(  Per spec window.performance is never null.

Alvin, how can I reproduce the problem you're seeing on airbnb?  I don't see it when I load the link in comment 2 in a clean profile in Firefox 31 on either Mac or Linux...

Comment 19

[Boris Zbarsky [:bzbarsky]]

And the reason I'd like to figure out how to reproduce is because I have at best 30% confidence right now that the fix for Philip's issue will also fix airbnb.

Comment 20

[Alvin Sng]

Hey Boris,

We've already pushed a change on our side to wrap the call in a try/catch block. We did not dig deep enough to generate a minimal test html file to properly reproduce the issue. It seems like it may have been fixed by Firefox already but we are not sure.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Alvin, is there a page you can point me to that would still reproduce the issue?  It doesn't have to be minimal, just reliable.

> It seems like it may have been fixed by Firefox already 

Definitely not.  I'm trying to make sure we _do_ fix it, but I simply have nothing to go on for your stuff: no links that show the problem, nothing.  :(

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional fix to not clobber existing mTiming value

Try caught this: if we have an mTiming already when entering CreateAboutBlankContentViewer, we don't want to null it out after creating the blank viewer.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up patch

Comment 24

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b960226e0b

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8466880 [details] [diff] [review]
Roll-up patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1025078
[User impact if declined]: Some web pages may not work right because of
   unexpected exceptions.
[Describe test coverage new/current, TBPL]: Test coverage is not perfect
   (e.g. bug 748276 has no regression test as far as I can tell) but does exist,
   as you can tell by the first (buggy) patch version failing tests.
[Risks and why]: Risk is moderate.  I'm fairly certain that this change is
   correct, but not certain enough to stake someone's life on: this is fairly
   complicated code.  I do think the risk of regressions that are worse than
   this problem is pretty low, because the problem is fairly bad.
[String/UUID change made/needed]: None.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8466880 [details] [diff] [review]
Roll-up patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
   This is a pretty serious web compat regression.
User impact if declined: Some web pages may not work right because of
   unexpected exceptions.
Fix Landed on Version: 34.
Risk to taking this patch (and alternatives if risky): Risk is moderate.
   I'm fairly certain that this change is correct, but not certain enough to
   stake someone's life on: this is fairly complicated code.  I do think the
   risk of regressions that are worse than this problem is pretty low, because
   the problem is fairly bad.  An alternative is to back out the fix for bug
   1025078 and refix it in a different way (by adding a null-check instead). 
   That would restore exactly the behavior we had in Firefox 30, which is
   suboptimal, I believe worse than the behavior with this patch, but better
   than what we have now.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 27

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/d1b960226e0b

Comment 28

[Preeti Raghunath(:Preeti)]

Andrew,

Please help understand:
1) User impact
2) Performance regression
3) blocker or not from your perspective

Comment 29

[Andrew Overholt [:overholt]]

I will defer to Boris' comment 25 and comment 26.

Comment 30

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/48788ecea2c6
https://hg.mozilla.org/releases/mozilla-beta/rev/70277dbb9071
https://hg.mozilla.org/releases/mozilla-esr31/rev/9770d4230573

Comment 31

[Jason Smith [:jsmith]]

Blocking for the compatibility concern.

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/70277dbb9071