883592 - nsScriptLoader::EvaluateScript sometimes tries to evaluate scripts for documents with no inner windows

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

This is the cause of bug 861700. See comments there for the debugging that led to filing this. I'm filing this separately to be able to work without the tbplbot spam.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Per bug 861700 comment 393, this is one way to avoid the problem. Maybe this check could be at a different level (haven't really studied he path to ProcessRequest()), but this solves the orange according to try.

Null push of current trunk: https://tbpl.mozilla.org/?tree=Try&rev=7492ebe933b7
* bug 861700 occurred immediately

Push with this patch: https://tbpl.mozilla.org/?tree=Try&rev=a43e3930f86e
* no failures in 20+ runs, it usually takes much less than that to trigger it, per previous try pushes:
  - https://tbpl.mozilla.org/?tree=Try&rev=3cda13682a35
  - https://tbpl.mozilla.org/?tree=Try&rev=5e9cfc0e9f95
  - https://tbpl.mozilla.org/?tree=Try&rev=0a5e0cd2c54a

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 763194 [details] [diff] [review]
patch

Jonas, is skipping the ScriptAvailable/ScriptEvaluated calls here OK?  Especially for XSLT?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I spoke to sicking on IRC, he wasn't confident that it's OK to skip the FireScriptAvailable/FireScriptEvaluated notifications in this case. So I'm going to prepare a more conservative patch that only skips the same stuff we'd skip if the beforescriptexecute event was preventDefault()ed.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 5

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19d5caf61217

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/19d5caf61217

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this something that would be worth uplifting to 24?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I don't see any reason to; short of causing some errors to throw off tests, there aren't really any negative effects here. Are you asking because bug 861700 is annoying?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

I uplifted the fix for bug 861700 et al to beta this morning. Was just going through the list and wondered if this had any other usefulness.

Comment 10

[u279076]

Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7)
> Is this something that would be worth uplifting to 24?

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I don't see any reason to

Seems like we should. In rare cases it can cause random crashes and there is no risk taking it...

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Which random crashes? As far as I know the negative effects of this bug was limited to not-so-useful error events.

Comment 13

[Gabor Krizsanits (INACTIVE)]

David, do you think that the crash that you described in bug 916788 might happen without your patch queue? As in... do you think it worth to uplift this patch to avoid random crashes like the one you ran into?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

The crash in bug 916788 was the same issue as bug 897399 (a regression from this bug's patch), caused by async script loading (bug 906371), and fixed in bug 915625. The patch in this bug doesn't fix any crashes, just wasted work.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Indeed, I mixed up two bugs hence my confusion, sorry for wasting your time.