Bug 24449 – Bubble Spinner game page crashes

• Component: Page Loading
• Status: RESOLVED DUPLICATE

[Cameron Zwarich (cpst)]

The page <http://www.addictinggames.com/bubblespinner.html> crashes with wildly different backtraces. It has nothing to do with playing the game, and some say it even happens with a "Click to Flash" input manager hack.

[Cameron Zwarich (cpst)]

This also crashes with plugins disabled.

[Cameron Zwarich (cpst)]

I can reproduce with a local copy and <base href=...>, so I should be able to make a reduction. It seems to crash a lot in DocLoader::setLoadInprogress() under Loader::Host::didFinishLoading() trying to get the Frame of a null document.

[Cameron Zwarich (cpst)]

The problem seems to be due to this piece of JS, because it goes away if I delete it:

$(function() {
                                                  setTimeout('update_ad_iframe("iframe3","http://ad.doubleclick.net/adj/ag.nol/puzzlesboards/runofsection_puzzlesboards;gw=puzzlesboards;sec_0=runofsection_puzzlesboards;!category=expand;u|gw-puzzlesboards|sec_0-runofsection_puzzlesboards|;",4,160,600,10 * 60 * 1000,"no")',1600);
				        });	

I imagine something is going wrong with loading the new content in the ad iframe.

[Cameron Zwarich (cpst)]

Created attachment 28396 [details]
Partial reduction

This is a small reduction, but it still uses the JS libraries on their site. The problem is caused by them having two timers to update the iframe set to the same timeout (presumedly due to undesirable code duplication on their part). These timers are made at different times during parsing, so they are a bit apart. It seems that canceling the request caused by the first timer and processing the request caused by the second timer. Presumedly, the crash is caused by using objects from the first partially completed load when processing the second load, but I am not sure.

I make this crash consistently for me by opening it up in a bunch of new tabs. Usually when I get to 4 or 5 it will crash.

[Cameron Zwarich (cpst)]

Created attachment 28398 [details]
Assertion failure stack trace

I was waiting on a fresh debug build. It turns out my reduction consistently causes an assertion failure.

[Cameron Zwarich (cpst)]

Created attachment 28400 [details]
Reduction

Here is a simple reduction for the assertion failure. It still loads some content from the network, but it is not much and is essential to the reduction.

There is no problem with setting the src of an iframe in quick succession with timers. However, when it is done using a javascript: URL that returns a constant string for the source of the iframe that then references a script file over the network, it causes the assertion failure. I don't really understand why, but I will look into it.

[Mark Rowe (bdash)]

<rdar://problem/6658053>

[Cameron Zwarich (cpst)]

Created attachment 28401 [details]
Simpler reduction

Here is a simpler reduction involving only one external resource, the SquirrelFish PNG. ;-)

[Cameron Zwarich (cpst)]

Comment on attachment 28401 [details]
Simpler reduction

Strange, this simpler reduction no longer works for me.

[Cameron Zwarich (cpst)]

The problem is that loading a JS URL doesn't cancel pending requests. Adding a call to stopLoading() in FrameLoader::executeIfJavaScriptURL() fixes the bug, because of this section of code in stopLoading():

            cache()->loader()->cancelRequests(docLoader);

However, this is probably not the right change. Hopefully someone more familiar with FrameLoader can let me know what to do here.

[Cameron Zwarich (cpst)]

Created attachment 28404 [details]
Proposed patch

Here's a patch that fixes the problem. It uses stopAllLoaders(), because that seems to be what is used in this situation.

I am confident that I can make a DRT HTTP test that reproduces this bug, but it might take me a while, so I am putting the patch up for review first.

[Cameron Zwarich (cpst)]

Comment on attachment 28404 [details]
Proposed patch

Doh. I posted my old diff. ;-)

[Cameron Zwarich (cpst)]

Created attachment 28405 [details]
Revised proposed patch

[Alexey Proskuryakov]

See also: bug 23736 (duplicate?)

The fix looks reasonable, but it's no good without a test.

[Cameron Zwarich (cpst)]

I am marking this a dupe of bug 23736, because my patch here also fixes that bug and the discussion there is both longer and involving more people.

*** This bug has been marked as a duplicate of 23736 ***

[Cameron Zwarich (cpst)]

Comment on attachment 28405 [details]
Revised proposed patch

Clearing review because it would be confusing on a dupe.