Last Comment Bug 637160 - window-modal prompts used instead of tab-modal prompts when reentering a page using back/forward
: window-modal prompts used instead of tab-modal prompts when reentering a page...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla6
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
data:text/html;charset=utf-8,%3Cinput...
: 647577 648236 648627 650301 673099 (view as bug list)
Depends on:
Blocks: abp 59314
  Show dependency treegraph
 
Reported: 2011-02-27 08:37 PST by Dan Wolff
Modified: 2011-09-02 05:07 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimal content policy extension for testing (1.40 KB, application/x-xpinstall)
2011-02-27 13:25 PST, Wladimir Palant
no flags Details
Minimal content policy extension for testing (1.47 KB, application/x-xpinstall)
2011-02-28 01:05 PST, Wladimir Palant
no flags Details
Patch v.1 (721 bytes, patch)
2011-02-28 19:59 PST, Justin Dolske [:Dolske]
bzbarsky: review+
Details | Diff | Review
testcase (277 bytes, text/html)
2011-04-11 13:26 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch v.2 (764 bytes, patch)
2011-04-15 18:28 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review

Description Dan Wolff 2011-02-27 08:37:46 PST
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

Reentering a page using the back button causes the new content-modal dialogs for the JavaScript alert() and confirm() functions to be blocked, apparently defaulting back to the previous window-modal dialogs.

Reproducible: Always

Steps to Reproduce:
1. Go to a web page which uses alert() or confirm() (e.g. http://www.tizag.com/javascriptT/javascriptconfirm.php)
2. Go to another web page
3. Go back to the original page (e.g. by pressing backspace)
4. Trigger the alert() or confirm()
Actual Results:  
A window-modal dialog opens.

Expected Results:  
The content-/tab-modal dialog should open.
Comment 1 Dave Garrett 2011-02-27 12:51:31 PST
I can reproduce using Minefield on Linux using my main profile, but can't reproduce it in a new profile on Linux or Windows.

More precise STR (especially because I have backspace not as back):
1: go to http://www.tizag.com/javascriptT/javascriptconfirm.php
2: scroll down a bit and click the button labeled "Leave Tizag.com"
3: click ok to the tab-modal prompts twice
4: script sends you to Google
5: click back and repeat step 2
6: prompts are now the old modal dialogs, instead of tab-modal prompts
Comment 2 gadjo 2011-02-27 12:54:06 PST
Dupe of bug 629911?
Comment 3 Dave Garrett 2011-02-27 13:04:53 PST
(In reply to comment #2)
This looks nothing like that.

I've managed to reproduce this on Linux and Windows starting from a new profile now. The missing ingredient: install Adblock Plus 1.3.3

CCing Wladimir. I'm worried that somehow Adblock might accidentally be causing web JS to run in a different context that gets out of the tab-modal prompts. Worst-case scenario would be if it causes it to be run with chrome privileges.

Notes: Also tried with latest Adblock Plus dev build with same results. Used Easylist subscription with tests.
Comment 4 Wladimir Palant 2011-02-27 13:25:29 PST
Created attachment 515510 [details]
Minimal content policy extension for testing

I can confirm this issue. However, it has nothing to do with Adblock Plus - merely with a presence of a JavaScript-based content policy. The same thing is happening with my minimal content policy extension as well, I attached it to this bug. Unlike Adblock Plus this extension doesn't do anything other than registering a content policy and dumping incoming data to console.
Comment 5 Dave Garrett 2011-02-27 13:39:59 PST
Content policies was were I would expect the problem to be coming from.

I can't seem to reproduce it using the attached test extension, however. Anything extra I need to do besides installing it into a new profile?
Comment 6 Alex Lakatos[:AlexLakatos] 2011-02-27 23:42:07 PST
Works For Me with a clean profile on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

Works For Me with Adblock Plus 1.3.3 installed on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Comment 7 Wladimir Palant 2011-02-28 01:05:50 PST
Created attachment 515575 [details]
Minimal content policy extension for testing

My bad, I forgot that the content policy extension in my profile was a modified version. Attached the modified version now, verified that it will cause the issue on a clean profile (Minefield build 20110227 on Windows 7). It seems that having a content policy is not enough, one also has to block a script on the page - and that's what this extension is doing now (blocking http://pagead2.googlesyndication.com/pagead/show_ads.js request).
Comment 8 Dave Garrett 2011-02-28 11:12:13 PST
Ah, ok. Now I can reproduce using your second testcase extension. Thanks.

I'll confirm this and move it over to Core:General. Not sure exactly what component it belongs in, though.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-28 11:14:23 PST
Justin, any idea what's going on here?  Are we somehow getting confused about which tab the dialog belongs to?
Comment 10 Dave Garrett 2011-02-28 11:36:26 PST
I reduced the content JS part of the testcase and put it in a data URI above. Note that this is reproducible with just one confirm(), or one alert(), or one prompt(). Same effect for each: navigate to another page and back and then it'll be window-modal instead of content-modal.
Comment 11 Dave Garrett 2011-02-28 11:44:04 PST
Just to be clear, the test extension is blocking "/show_ads.js" but the test content JS doesn't need to have the file being blocked. I can still reproduce with it with only a button for a prompt and the test extension.
Comment 12 Justin Dolske [:Dolske] 2011-02-28 18:35:53 PST
(In reply to comment #9)
> Justin, any idea what's going on here?  Are we somehow getting confused about
> which tab the dialog belongs to?

Hmm. I'd actually guess this is a result of the code added in bug 613800, which forces use of window-modal prompts in some cases (ie, when a tab-modal prompt could result in unexpected reentrancy).

Still seems unexpected here, though, so maybe some of the existing accounting which 613800 checks is incorrect?

I can reproduce this with my normal profile on OS X (which has Adblock), but not yet on my Windows box. Lemme try again with the STR from the last few comments.
Comment 13 Justin Dolske [:Dolske] 2011-02-28 18:44:27 PST
I can reproduce on Windows now.

In DocumentViewerImpl::GetIsTabModalPromptAllowed(), mHidden is |true|. Which I guess isn't actually surprising, because there doesn't seem to be any code to set it back to |false| when a previous-hidden page is no longer hidden. Sigh.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-28 18:51:16 PST
Er, yeah.  Looks like Show() should set mHidden to false?  I wonder why that ever worked...
Comment 15 Justin Dolske [:Dolske] 2011-02-28 19:59:34 PST
Created attachment 515826 [details] [diff] [review]
Patch v.1

This fixes the problem for me.

Any other places that mHidden should be reset? I don't know this code well enough to say, but this seems like the obvious missing spot.

::Stop() is the only other place mHidden is checked [other than ::GetIsTabModalPromptAllowed()], so this seems safe enough.

Hmm, why do the STR need a content policy to trigger this? I don't see an obvious connection, does the policy have some effect on bfcache / when things are shown or hidden?
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-28 20:07:10 PST
> Hmm, why do the STR need a content policy to trigger this?

This part I don't know.  It seems like any time a page comes from bfcache we should hit this problem, no?
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-28 20:21:40 PST
Comment on attachment 515826 [details] [diff] [review]
Patch v.1

Does putting this in Open() also work?  It would make a bit more sense there, I think...  r=me either way.
Comment 18 Justin Dolske [:Dolske] 2011-04-03 14:15:08 PDT
*** Bug 647577 has been marked as a duplicate of this bug. ***
Comment 19 Justin Dolske [:Dolske] 2011-04-08 11:54:16 PDT
*** Bug 648236 has been marked as a duplicate of this bug. ***
Comment 20 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-04-11 13:26:05 PDT
Created attachment 525148 [details]
testcase

I'm seeing it also in this case, when adding an alert in a pageshow event. I guess the patch will fix the issue.
Comment 21 Masatoshi Kimura [:emk] 2011-04-15 15:51:51 PDT
*** Bug 650301 has been marked as a duplicate of this bug. ***
Comment 22 Masatoshi Kimura [:emk] 2011-04-15 15:54:45 PDT
Why is this patch not landed yet?
Comment 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-04-15 17:44:26 PDT
Note that there is a pending review comment on the patch.  Please don't check it in without addressing that comment.
Comment 24 Justin Dolske [:Dolske] 2011-04-15 17:48:13 PDT
Exactly what I was going to say when you midaired me! :)
Comment 25 Justin Dolske [:Dolske] 2011-04-15 18:03:47 PDT
*** Bug 648627 has been marked as a duplicate of this bug. ***
Comment 26 Justin Dolske [:Dolske] 2011-04-15 18:28:58 PDT
Created attachment 526444 [details] [diff] [review]
Patch v.2

Moved mHidden reset from ::Show() to ::Open(), still fixes bug.
Comment 27 Justin Dolske [:Dolske] 2011-04-15 18:30:47 PDT
Adjusting summary, since I don't see anything to indicate a content policy is actually required to trigger this bug. Feel free to retest after this lands, and if there's still some remaining issue involving a content policy, let's file a new bug (and just refer to this one for some of the history).
Comment 28 Justin Dolske [:Dolske] 2011-04-19 14:21:21 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/4aaf0034ce82
Comment 29 Justin Dolske [:Dolske] 2011-07-24 23:26:01 PDT
*** Bug 673099 has been marked as a duplicate of this bug. ***
Comment 30 patrickjdempsey 2011-09-02 05:07:27 PDT
Seeing this in the latest 7.0 beta with AdBlock Plus installed.  Can confirm that disabling ABP and reloading the page fixes the problem.

Note You need to log in before you can comment on or make changes to this bug.