14599 - editor needs to detect attempt to load frameset document

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Michael Schumann]

when openning an HTML page with a frameset tag, the editor freezes.

Comment 1

[buster]

we can't create framesets, and for beta1 you won't be able to launch the editor
on an arbitrary document (via "edit page" or "file open" or ...).  So this bug
can wait until after beta entry.  In fact, I'm pretty sure it's a duplicate.

Comment 2

[buster]

composer is in, so this is more important now.  marking it a critical P1
crasher.

Comment 3

[rubydoo123]

using the 19991102 build, the editor doesn't freeze, it seems like the entire
desktop freezes. I had to select Ctrl+Alt+Delete and shut down the app to regain
control. This is quite a nasty little bug.

Comment 4

[buster]

I have already started looking into this.  I have a partial fix (the app doesn't
lock up or crash), but it's not 100% correct yet, because the document does
still load.  We'd rather pop up a message saying "sorry, bub, you don't get to
edit frameset documents this year."  When M12 opens, I'll check in (at least)
the partial fix.

Comment 5

[buster]

crasher, not marked dogfood.   seems redundant to mark crashers as dogfood,
since crashing clearly prevents use!  but some crashers are worse than others.
The workaround is simply "don't open framesets", but the user doesn't always
know what is a frameset and what isn't.  So "edit page" can crash them
unexpectedly.

Comment 6

[leger]

Not needed for Dogfood...putting on PDT- radar....will Release Note feature
problem for dogfood.  Changed [DOGFOOD] in summary to [BETA]

Comment 7

[buster]

fixed the crash, but leaving bug open because we still don't do 100% the right
thing.
The frameset loads in the editor window, but no editor is actually attached, so
no editing is possible.  Next step is to detect the error case and stop the
document load altogether.
removed [CRASH} and [PDT-], lowering priority and severity and pushing out
milestone.

Comment 8

[buster]

changed summary to reflect the partial fix I have in place.

Comment 9

[buster]

cc-ing people who know about URL loading:  jud, travis, scott.

I need a way of find what the doctype is early in the loading process, so I can
abort the load and put up a dialog or whatever.  The point is, we don't want to
load a document we can't edit, and we're JUST and HTML editor for now.

If the editor sets itself up as an nsIDocumentLoaderObserver, is
OnStartDocumentLoad() the right place?  How do I ask the question "what is the
doctype of the doc you're about to load?"

Thanks

Comment 10

[Scott MacGregor]

buster, by doc type do you mean the content type of the document?

Comment 11

[buster]

yeah:  I need to differentiate between HTML and XML for starters.  I should be
able to differentiate between normal HTML and FRAMESET documents as well.

Comment 12

[Judson Valeski]

it sounds like you shouldn't even be envoked if you can't handle a doc. rather
than spitting it back if you can't handle it, the URI loader shouldn't hand it
to you in the first place. I'm assigning this to rpotts as he's spearheading the
URI targeting right now.

Comment 13

[rpotts (gone)]

Oh great...  As if life wasn't sucky enough in URI targeting land - you had to
give this one to me :-)

I'm not sure if this is even a URI targeting issue at all - I'm sure you were
just waiting for me to say that!!

But, really, HTML doesn't have much in the way of structure...  So, the only
time you'll be *sure* that you don't have a FRAMESET document is *after* the
entire document has been parsed!  This is way past the URI targetting stage...

It seems like a better approach would be to register as a tag observer on the
parser and fail out of the load if a FRAMESET or XML document is encountered...

make any sense?
-- rick

Comment 14

[rpotts (gone)]

I'm kicking this one back to buster :-)  I don't think that URI targeting is
sufficient to fix the problem...

-- rick

Comment 15

[buster]

marking potential beta1 bugs

Comment 16

[leger]

PDT+

Comment 17

[buster]

assigning to rickg.  he needs to put in some infrastructure before the editor 
can do anything.

Comment 18

[rickg]

Steve -- Why does the parser need to do anything for an XML document? You 
already know the document type due to the actual document object that gets 
constructed. I'm proceeding on the basis that this is just a frameset issue.

Comment 19

[buster]

rick: what you said.  I know how to handle xml.  it's just framesets where I 
need parser/document support.

Comment 20

[rickg]

Buster: Another wonderment I have is why we want to put this interface on the 
contentsink, rather than use the tag observer mechanism that I already have. In 
essence, you would register as an observer of <frameset>, and I'll call you if I 
see it. You can return a status code to me to tell me to stop loading if that's 
the right answer (we already do that in charset detection). 

Doesn't that solve this issue for you?

Comment 21

[rickg]

After discussing with buster, I believe that the parsing engine already has all 
the functionality we need. Steve: get with me to learn how to use this feature; 
it's really, REALLY easy.

Comment 22

[buster]

the parser tag observer works great for framesets.  but not at all for xml.
I'm going to split up this bug into several pieces:
1) editor needs to detect attempt to load frameset document (this bug)
2) editor needs to detect attempt to load xml document (bug 28972)
3) the editor UI needs to handle illegal document types (bug 28973)
4) the browser "Edit Page" action should be disabled if the current document 
loaded in the browser is a frameset or xml document (bug 28974)

PDT Team: I've nominated all of the derivative bugs, because it's not clear 
which part(s) of this are considered beta blockers.

Comment 23

[buster]

Attachment: changed code for fix, you also need header file to be attached in a moment...

Comment 24

[buster]

Attachment: new header file, no change to make files required.

Comment 25

[buster]

in the patch I posted, travis suggests changing "aLoader" to "mWebShell" for the 
Stop() method.  Tested, and this works fine as well.

Comment 26

[Jim Roskind]

When do we expect to see the landing?  Please update the status whiteboard, or
even better... close the bug as fixed ;-)
Thanks :-)

Comment 27

[buster]

since this code has no user-visible effect, and the bugs that depend on it are 
PDT-, I think this should be PDT- as well. Removed PDT+ for reconsideration.

Comment 28

[Phil Peterson]

PDT-

Comment 29

[buster]

simon told me yesterday he was taking this one.  Working but partial fix is 
attached.  Three things still need to get done:
1) nsHTMLEditorParserObserver needs to support the weak reference API, so the 
parser can release it properly.  
FYI: RickG has a bug about how the parser releases these listeners which only 
effects listeners that register for more than one tag, so I think we're 
uneffected by that bug here since we only listen for <frameset>.
2) We need to decide what exactly to do in response to a frameset notification.  
Stop loading the document? Pop up a dialog? Bring up the editor window or kill 
it entirely? Charley has a bug on this part of it, I think
3) Different editor types would need to provide their own parser listeners. We 
need to decide how the shell gets the parser listener.  It should probably be 
dependent on the editor type, maybe even provided by the editor itself through 
some generic editor interface.  Except as I recall the parser listener is needed 
before the editor is actually instantiated, so maybe it's a static on the 
interface?

Like I said yesterday, sniffing for XML is an entirely different problem.  The 
parser observer is no help there.  mscott or rick potts can point you in the 
right direction for getting the "xml" out of the data stream from the doc 
loader, I hope.

Comment 30

[Kathleen :Brade]

move to M15 since this didn't make beta1 (PDT-)

Comment 31

[Simon Fraser [no longer active]]

Some investigation as to why frameset documents continually reload in the editor.
The reload happens because, for some reason, nsHTMLFrameInnerFrame::Reflow() is 
called in such a way that it decides to call LoadURI to reload the frame 
documents over again. This only happens if we've made the 
PrepareDocumentForEditing() call in the editor. So something weird is happening 
in that call.

Comment 32

[Simon Fraser [no longer active]]

It's the call to 
  styleSheets->ApplyOverrideStyleSheet("chrome://editor/content/
EditorOverride.css");
that is causing the trouble here. cc: charlie.

Comment 33

[Simon Fraser [no longer active]]

I checked some code in here, but it still needs work. The changes I checked in do 
two things:
1. If we let a frameset load in editor, the editor will not crash, but will end 
up sitting on the last document to load (which is actually editable).
2. There is code to detect a frameset tag, and an alert is shown if one is 
detected. We don't yet properly handle things from then on (since closing the 
window programmatically at that point crashes).

Comment 34

[Simon Fraser [no longer active]]

Fix checked in. We now show a dialog (just one) if you try to edit a frameset, 
then close the window.

Comment 35

[sujay]

I just tried loading that page above using 4/4 build..I don't see any dialog
come up...instead I see the editor attempting to open that page with
frameset, the frame in the middle of the page keeps blinking...

reopening...

Comment 36

[sujay]

ok I was uysing the wrong build.....but I tried it
again on 4/4 build and now we crash after clicking OK on that
framset panel.

Comment 37

[Simon Fraser [no longer active]]

This is actually fixed; we do detect the frameset now, and throw up the dialog 
OK. It crashes, but sujay is filing a separate bug on that.

Comment 38

[sujay]

verified in 4/4 build.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b48ee140c10
[fenix] Import l10n. (https://github.com/mozilla-mobile/fenix/pull/14599)