iframes DOMContentLoaded events can interfere with downloadAndParseDocument()

VERIFIED FIXED in Firefox 16

Status

()

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bnicholson, Unassigned)

Tracking

unspecified
Firefox 17
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16- verified, firefox17 verified, firefox18 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
STR:
1) Go to http://www.cnn.com/2012/08/09/living/olympians-bite-medals/index.html?hpt=hp_c2
2) Wait until the reader icon appears
3) Click the icon

Expected result:
Parsed page appears

Actual result:
Failed to load article from page error

This happens because the listener in _downloadDocument() is catching the DOMContentLoaded event for the iframe. I see the following when I try to parse that page:

08-10 10:46:52.003 28541 28557 E GeckoConsole: BRN: DOMContentLoaded: http://www.cnn.com/2012/08/09/living/olympians-bite-medals/index.html?hpt=hp_c2
08-10 10:46:52.534 28541 28557 E GeckoConsole: BRN: DOMContentLoaded: http://svcs.cnn.com/weather/getForecast?time=24&mode=json_html&zipCode=30303&locCode=&celcius=false&csiID=csi2

This means the callback is fired twice, and since the second page is an iframe, it returns null. This, in turn, fires runCallbacksAndFinish() with the failed result.
Created attachment 651481 [details] [diff] [review]
Ignore DOMContentLoaded events from iframes in Reader
Attachment #651481 - Flags: review?
Attachment #651481 - Flags: review? → review+

Updated

6 years ago
tracking-firefox16: --- → ?
(Reporter)

Comment 2

6 years ago
What is the "doc.defaultView.frameElement" in this line supposed to be checking?

       if (doc.location.href == "about:blank" || doc.defaultView.frameElement) {

I thought that was already checking to see if we were in an iframe. If not, could you add a comment describing what it's for?
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> What is the "doc.defaultView.frameElement" in this line supposed to be
> checking?
> 
>        if (doc.location.href == "about:blank" ||
> doc.defaultView.frameElement) {
> 
> I thought that was already checking to see if we were in an iframe. If not,
> could you add a comment describing what it's for?

The current check would only work if we were listening to "load" events but we're listening to "DOMContentLoaded" events. We can safely remove the current check. I updated the patch to do so.
tracking-firefox16: ? → -
https://hg.mozilla.org/mozilla-central/rev/9493806638d0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Comment on attachment 651481 [details] [diff] [review]
Ignore DOMContentLoaded events from iframes in Reader

[Approval Request Comment]
User impact if declined: Reader will fail to load content on pages with iframes (i.e. any page with iframe ads). e.g. CNN, arstechnica, and more.
Testing completed (on m-c, etc.): A few days in m-c, no regressions.
Risk to taking this patch (and alternatives if risky): Very low, simple fix. Just properly handling of DOMContentLoaded events in iframes.
String or UUID changes made by this patch: None.
Attachment #651481 - Flags: approval-mozilla-aurora?
Attachment #651481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
status-firefox16: --- → fixed

Comment 8

6 years ago
Verified on:
Firefox Mobile 16 beta 2, Nightly 18.0a1 2012-09-04 and Aurora 17.0a2 2012-09-04 
Samsung Galaxy Tab (Android 2.3.3)
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
status-firefox17: --- → verified
status-firefox18: --- → verified
You need to log in before you can comment on or make changes to this bug.