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

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Philip Tellis, Assigned: bz, NeedInfo)

Tracking

31 Branch
mozilla34
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32+ fixed, firefox33+ fixed, firefox34+ fixed, firefox-esr24 unaffected, firefox-esr31 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8463438 [details]
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)
(Reporter)

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

4 years ago
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.
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...
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Product: Firefox → Core
(Reporter)

Comment 6

4 years ago
@Alvin, instead of removing the code, I've found that wrapping it in a try/catch is sufficient to suppress the error on Firefox.
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.
Blocks: 1025078
blocking-b2g: --- → 2.0?
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
Created attachment 8466286 [details]
More minimal testcase
Attachment #8466286 - Attachment description: test.html → More minimal testcase

Updated

4 years ago
Duplicate of this bug: 1047451
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?
Flags: needinfo?(sng.alvin)
Flags: needinfo?(philip.tellis)
(Reporter)

Comment 11

4 years ago
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.
Flags: needinfo?(philip.tellis)
Summary: window.performance throws NS_ERROR_FAILURE in anonymous iframes → window.performance throws NS_ERROR_FAILURE in iframes created with a javascript: URI if touched before the javascript: returns a string
OK, so that code is a no-op if window.performance returns null.  That explains why it was "working" before.
(Reporter)

Comment 13

4 years ago
yes.  doesn't everyone do capability checks in JavaScript before using a capability ;)
Created attachment 8466304 [details] [diff] [review]
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
Attachment #8466304 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
> 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.  ;)
Whiteboard: [need review]
(Reporter)

Comment 16

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

Comment 17

4 years ago
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.
Flags: needinfo?(sng.alvin)

Updated

4 years ago
Attachment #8466304 - Flags: review?(bugs) → review+
> 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...
Flags: needinfo?(sng.alvin)
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

4 years ago
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.
Flags: needinfo?(sng.alvin)
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.  :(
Flags: needinfo?(sng.alvin)
Created attachment 8466606 [details] [diff] [review]
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.
Attachment #8466606 - Flags: review?(bugs)

Updated

4 years ago
Attachment #8466606 - Flags: review?(bugs) → review+
Attachment #8466304 - Attachment is obsolete: true
Flags: in-testsuite+
Attachment #8466606 - Attachment is obsolete: true
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.
Attachment #8466880 - Flags: approval-mozilla-beta?
Attachment #8466880 - Flags: approval-mozilla-aurora?
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.
Attachment #8466880 - Flags: approval-mozilla-esr31?
status-firefox31: --- → wontfix
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr24: --- → unaffected
status-firefox-esr31: --- → affected
tracking-firefox32: ? → +
tracking-firefox33: ? → +
tracking-firefox34: --- → +
https://hg.mozilla.org/mozilla-central/rev/d1b960226e0b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Andrew,

Please help understand:
1) User impact
2) Performance regression
3) blocker or not from your perspective
Flags: needinfo?(overholt)
I will defer to Boris' comment 25 and comment 26.
Flags: needinfo?(overholt)
Attachment #8466880 - Flags: approval-mozilla-esr31?
Attachment #8466880 - Flags: approval-mozilla-esr31+
Attachment #8466880 - Flags: approval-mozilla-beta?
Attachment #8466880 - Flags: approval-mozilla-beta+
Attachment #8466880 - Flags: approval-mozilla-aurora?
Attachment #8466880 - Flags: approval-mozilla-aurora+
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
status-b2g-v2.1: affected → fixed
status-firefox32: affected → fixed
status-firefox33: affected → fixed
status-firefox-esr31: affected → fixed
Blocking for the compatibility concern.
blocking-b2g: 2.0? → 2.0+
You need to log in before you can comment on or make changes to this bug.