491063 - Mixed mode warning dialog (and other dialog windows) is sometimes not rendered

Status: RESOLVED FIXED

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

Description

[Frank Wein [:mcsmurf]]

To reproduce:
0. Fetch Firefox trunk or 1.9.1 build
1. Make sure security.warn_viewing_mixed is set to true
2. Go to https://wiki.mozilla.org/Platform/2009-04-21#Blocker_Report and click on one of those bugzilla links
3. As soon as the bugzilla query has loaded, click the "Back" button in the browser and observe an empty warning dialog
(4. Click on "Forward" and "Back" again, now the warning should no longer be empty)

Note that Step 4 does not always seem to work, it works in a Firefox trunk build, but with a SeaMonkey 1.9.1 build, it's still broken at this point.

Comment 1

[Frank Wein [:mcsmurf]]

It looks like this might be a regression from Bug 466057:
Works fine with http://hg.mozilla.org/mozilla-central/rev/050ae62b7d9d, broken with http://hg.mozilla.org/mozilla-central/rev/b1251b9b986e range: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-12-19+20%3A12%3A00&enddate=2008-12-21
Works fine with http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e344268b7fb, broken with http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6b33245bbd2a, range: http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?startdate=2009-01-06+19%3A00%3A00&enddate=2009-01-08

Comment 2

[Frank Wein [:mcsmurf]]

Attachment: Screenshot

Comment 3

[Frank Wein [:mcsmurf]]

See http://mxr.mozilla.org/comm-central/source/mozilla/security/manager/boot/src/nsSecurityWarningDialogs.cpp#128 for the code where the dialog gets displayed.

Comment 4

[Frank Wein [:mcsmurf]]

With a SeaMonkey 1.9.1 based build I get this assertion a few times when loading the Wiki page (with the working security dialog):
###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection == 1', file f:/mozilla/tree-hg/src/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp, line 613
and when pressing the Back button to go back to the Wiki page (with the broken security dialog):
###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file  f:/mozilla/tree-hg/src/mozilla/layout/base/nsDocumentViewer.cpp, line 1081

Comment 5

[memai]

related? Bug 474673

Comment 7

[Frank Wein [:mcsmurf]]

Moving to Layout because of the dependency and as this also occasionally happens with other dialog windows (for example with the "Warning: Unresponsive Script" dialog).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did this also regress on 1.9.0.*?  Is it SeaMonkey only or does it also affect Firefox?

Comment 9

[Frank Wein [:mcsmurf]]

Affects SeaMonkey and Firefox.
The 1.9.0.* branch is a bit difficult to test (need to find some test case first that brings up a warning). A 1.9.0.* build does not seem to care that the wiki page includes images from http://, it just ignores that (no warning).

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Smaug: you wrote the patch in bug 466057 that apparently caused this regression; can you look into it?

Comment 11

[Johnathan Nightingale [:johnath]]

To be clear - the broken behaviour of a non-default piece of UI is a bug to fix, but doesn't block the release on its own.  This is marked blocking so that we can ensure that this isn't the symptom of some larger bustage in mixed content detection or anything.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Johnathan, this is happening with multiple (possibly all modal, with varying difficulty of reproduction) dialogs.  See comment 7.  It's clearly a symptom of core dom or layout bustage _somewhere_.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I doubt all. I'll test this, but my guess is that this happens if we
open dialogs at "wrong" times.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I get also an assertion added in bug 401155, the one in DocumentViewerImpl::PermitUnload

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Probably similar to bug 482286 which shows a similar behavior for the cookies dialog?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Also the dialog for untrusted ssl certificates was shown blank for me a month ago while doing the FFT's.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
> ###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or
> OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection ==
> 1', file
> f:/mozilla/tree-hg/src/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,
> line 613
Kaie, any comments to this assertion?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #17)
> (In reply to comment #4)
> > ###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or
> > OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection ==
> > 1', file
> > f:/mozilla/tree-hg/src/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,
> > line 613
I think this might be a regression from something else than Bug 466057.
Would be great if someone could try to find that out, though it requires
debug builds.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Jonas, Boris, starting img/object/etc. synchronously in BindToTree causes
problems. I see two ways to fix this. One is to allow chrome flushes even
when there are scriptblockers (I don't like this approach) and the other
is to start loading using a script runner.
I just posted a patch to tryserver to see if that affects perf numbers,
though after that kind of change we could actually fix bug 467422 and
speed up generic Bind/Unbind.

I still have no idea what has caused nsSecureBrowserUIImpl.cpp assertions.
Maybe they've been there always.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: scriptrunners in bind

So something like this.
Jonas, Boris, any comments?

Comment 21

[Boris Zbarsky [:bzbarsky]]

I think using a script runner here is an excellent idea, generally speaking.  I assume that BindToTree already asserts that a script blocker is active on entry?

That said, for images in particular this can lead to issues, because of the sync loading stuff image loader can do.  In particular, there are cases where we'd lose big on perf if the load happens after frame construction, which will always be the case with the script runner, right?  I've been meaning to make that better (by assuming an image frame if the state is "still need to load" or something)...  maybe we should just do that?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I did post the patch to try server and at least that didn't show any perf problems.
Do you happen to know any perf testcases for img loading?

Comment 23

[Johnathan Nightingale [:johnath]]

(In reply to comment #22)
> I did post the patch to try server and at least that didn't show any perf
> problems.
> Do you happen to know any perf testcases for img loading?

I'd expect anything that did real damage to image loading would impact Tp3 pretty heavily since those pages are full of them, no?

Comment 24

[Boris Zbarsky [:bzbarsky]]

The performance issue I'm talking about happens when a page preloads an image and then has a script suddenly create lots of copies of it (e.g. using 1x1 pixel images to draw things on screen).  With the new code this operation would be O(N^2) in number of images as things stand.  None of our tinderbox tests exercise this (assuming they exercise image preloading at all, which I rather doubt).

Comment 25

[Boris Zbarsky [:bzbarsky]]

So basically it would make bug 455690 appear in a lot more cases.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

A testcase
http://mozilla.pettay.fi/moztests/1x1image.html

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Clear broken state if we're going to try to load something

This doesn't regress performance in the testcase. But I need to test this some more. Posted the patch to tryserver.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #26)
> A testcase
> http://mozilla.pettay.fi/moztests/1x1image.html

JFI: At least for me this test brought up an empty modal sheet which i wasn't able to close. The browser locked up.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with some IsInDoc checks

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 376996 [details] [diff] [review]
with some IsInDoc checks

Actually, we don't want IsInDoc checks to keep the old behavior

Comment 31

[Boris Zbarsky [:bzbarsky]]

That doesn't seem to address my concern from comment 21, does it?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

It does try to fix that.
comment 21 is basically http://mozilla.pettay.fi/moztests/1x1image.html, right?
And the patch clears broken state so that frame is created by default.
The first patch regress performance pretty badly with 1x1image.html.

Or is there some other case I'm not aware of?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  It clears broken but doesn't set loading, I guess.  So then we'll set loading when the runnable runs, and reframe at that point.  Which means that it slightly slows down the case when the image is _not_ preloaded (the normal pageload case).  I assume you did run that last patch through Tp?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #33)
> I assume you did run that last patch through Tp?
Yes, I did post it to tryserver. Didn't see any difference

Comment 35

[Boris Zbarsky [:bzbarsky]]

OK.  Just looking at the images for now...  If the "src" attr is present but we can't create a URI object from it, this new code won't put the image in a broken state, will it?  That should be reftestable.  Try <img src="http://a b"> (with the space).

Same thing for cases when mLoadingEnabled is false in nsImageLoadingContent.  That ought to be mochitestable, at least.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #35)
> OK.  Just looking at the images for now...  If the "src" attr is present but we
> can't create a URI object from it, this new code won't put the image in a
> broken state, will it?
Ah, right. nsImageLoadingContent::LoadImage doesn't handle that case, though
seems like one could trigger it even now if one sets .src first to some valid
value and then to some invalid one.

Ok, I need to check mLoadingEnabled somewhere.
(nsImageLoadingContent isn't very nice code :/ )

Comment 37

[Boris Zbarsky [:bzbarsky]]

> if one sets .src first to some valid value and then to some invalid one.

In that case we continue showing the old image, more or less on purpose.

> (nsImageLoadingContent isn't very nice code :/ )

Patches accepted!  Its main problem is having to deal with the "sometimes I'm sync, sometimes not" behavior of imagelib, though.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better

Still no mochitests or reftest. Will do those tomorrow.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: With reftest

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378035 [details] [diff] [review]
With reftest

So overall, this looks really nice.  Some nits:

1)  NS_NEW_RUNNABLE_METHOD might be nice to use throughout (no need to repeat
    the class name then).  Either way is fine, though.
2)  This patch introduces a behavior change for images in the loading-disabled
    case: we used to fire an error event and now no longer do.  It might be
    better to post the runnable for images unconditionally and change the code
    it runs to something like:

    if (GetAttr(....) &&
        (NS_FAILED(LoadImage(....)) ||
	 !LoadingEnabled())) {
    }

    with a comment about making sure that LoadImage() happens before
    LoadingEnabled() is checked there.  Similar for image inputs, SVG images,
    etc.
3)  For HTML input, we should be checking that mType is NS_FORM_INPUT_IMAGE in
    MaybeLoadImage, just in case someone changes the type between BindToTree
    and this code running.
4)  We should file a followup bug (dependent on the HTML5 parser landing) to
    investigate removing the various image/object aNotify arguments, perhaps.
5)  We still have places where we start network loads while script blockers are
    active.  Loading of images from nsCSSDataBlock code comes to mind, and I
    seem to recall that user font loads behave the same way.  We should get
    followup bugs filed on these.

r+sr=bzbarsky with the nits picked.  Thanks for adding that reftest!

Comment 41

[Boris Zbarsky [:bzbarsky]]

Except with that indent fixed, of course.  Darn tabs.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Moving to content-land since that's where the patch is.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +nits

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 191

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

This is fixed, but I need to file the followups and
a bug about nsSecureBrowserUIImpl.cpp.

http://hg.mozilla.org/mozilla-central/rev/dbf96b1b73bd
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/40fab443e59b

Comment 46

[Lucas Adamski [:ladamski]]

I can still get this to repro in 3.5pre on https://intranet.mozilla.org/forum/
About 1/3 of the time when I click through to a thread (many of which have HTTP avatars), I get a completely blank dialog that locks up the browser, requiring restart. 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre

Comment 47

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

Removing fixed1.9.1 for now. How confident are we that the 1.9.1 patch fixed the original problem? The reftest presumably failed without the patch, so I guess it does fix *a* problem, just not sure if we're confident that it fixed the problem Frank and Lucas are seeing.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

https://intranet.mozilla.org/forum wasn't the testcase for this bug.
Please open a new one

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

I can reproduce the forum problem if I set 
security.warn_viewing_mixed to true and security.warn_viewing_mixed.show_once to false. Investigating.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Bug 494590 filed on the forums issue.