If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FIX]Midas demo. (designMode) fails to work properly after restoring with session restore

RESOLVED FIXED in Firefox 3 beta1

Status

()

Firefox
Session Restore
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Adam Guthrie, Assigned: bz)

Tracking

({regression, verified1.8.1.8})

Trunk
Firefox 3 beta1
x86
Windows XP
regression, verified1.8.1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5

This is spun off from bug 385882.

Steps to reproduce:
1. Tools > Options > Main > Choose "Show my windows and tabs from last time"
2. Go to http://www.mozilla.org/editor/midasdemo/ and input some text.
3. File > Quit
4. Reopen Firefox and try typing into the editor.

Actual results:
The text that was in the editor is NOT restored and you cannot type anything in the editor. An error is also in the Error Console:

Error: uncaught exception: Permission denied to set property HTMLDocument.designMode

Expected results:
Text is restored and I'm able to edit it. 

This regressed on BRANCH between 2007-07-11-03 and 2007-07-12-03: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-11+02&maxdate=2007-07-12+04&cvsroot=%2Fcvsroot.

This issue has been around on trunk since at least 2007-01-01-04, FWIW.
Confirmed, I see this in the error console:
Error: uncaught exception: Permission denied to set property HTMLDocument.designMode
(Reporter)

Comment 2

10 years ago
Aha! So I tracked down the regression range on trunk to between 2006-08-15-04 and 2006-08-16-04. Bug 332182 landed in this range as well as in the branch regression range I found, so I guess we've found our offender.
Blocks: 332182
(Reporter)

Updated

10 years ago
Blocks: 385882
This is a bug in session restore, more or less.  Its serialization and deserialization of history entries doesn't include the principal (exposed as .owner on the history entry since bug 337260 got fixed).  As a result, when it loads that session history (which seems to be how it handles restoration as far as I can tell), the permission end up wrong.  It's always had problems for data: and javascript: URIs; all that happened is that about:blank now has the same problems.

Chances are, just serializing out the URI of the principal and then creating a codebase principal when deserializing will "work" on branch in most cases.  On trunk, I've been working on making principals implement nsISerializable.  I assume that session restore can store binary blobs as needed?

I'd be happy to help out if needed, but I'd want to be pointed at the places where we do the actual serializing and deserializing in nsSessionStore.js.  Then we'd want to change _deserializeHistoryEntry to use create() like docshell does itself.
Component: Editor → Session Restore
Flags: blocking1.8.1.6?
Product: Core → Firefox
QA Contact: editor → session.restore
Version: 1.8 Branch → Trunk
Then again, brendan's been arguing for an alternate serialization format for principals.  If people can figure out what they want, we could do that too.
Depends on: 388128
Flags: blocking-firefox3?
Flags: blocking1.8.1.7?
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M8

Comment 5

10 years ago
(In reply to comment #3)
> I assume that session restore can store binary blobs as needed?

Since we support JSON serialization, you'd have to manage with strings, IEEE 754 numbers and arrays containing those. An encoded string should probably do the job.

> where we do the actual serializing and deserializing in nsSessionStore.js. 

That's really just _serializeHistoryEntry and _deserializeHistoryEntry which convert a "@mozilla.org/browser/session-history-entry;1" to a JS object and vice versa.
Depends on: 390474
Flags: blocking1.8.1.6?
Not blocking the 1.8 branch, but when there's a trunk fix please renominate for the branch.
Flags: blocking1.8.1.7? → wanted1.8.1.x+

Updated

10 years ago
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Depends on: 369566
Created attachment 280766 [details] [diff] [review]
Trunk fix

This patch requires the fix for bug 369566 to work.  It also fixes the bug the existing code had with truncating postdata at the first null, while I was here.

jst, could you review the session history part?

mconnor, could you review the session store part?
Attachment #280766 - Flags: superreview?(jst)
Attachment #280766 - Flags: review?(jst)
Attachment #280766 - Flags: review?(mconnor)
Created attachment 280767 [details] [diff] [review]
Branch patch

Doesn't really have much to do with the trunk patch, for what it's worth.
Attachment #280767 - Flags: superreview?(jst)
Attachment #280767 - Flags: review?(mconnor)
Attachment #280767 - Flags: review?(jst)
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: Midas demo. (designMode) fails to work properly after restoring with session restore → [FIX]Midas demo. (designMode) fails to work properly after restoring with session restore

Comment 9

10 years ago
Comment on attachment 280766 [details] [diff] [review]
Trunk fix

Thanks for taking this on, Boris.
My comment to the SessionStore bits:

>-        var postdata = stream.read(stream.available());

What's the reason for the seemingly unrelated postdata changes?

>+      catch (ex) { debug(ex); }

What are you expecting to catch here?

>-      stream.setData(aEntry.postdata, -1);
>+      var postdata = atob(aEntry.postdata);
>+      stream.setData(postdata, postdata.length);

This looks like it will break if there's postdata to be restored when Firefox 3 is started for the first time (having Firefox set to "restore the windows/tabs from last time").

I'd prefer if you used a different property (aEntry.postdata_b64 or similar) and fell back to Firefox 2's property if available.

BTW: What's the advantage of base64'ing the string? Aren't JavaScript strings able to contain \0 ?

>+      var binaryData = atob(aEntry.owner);

Again: what will happen here when aEntry.owner is a nsIURI.spec from Firefox 2?
> What's the reason for the seemingly unrelated postdata changes?

See comment 7.

> What are you expecting to catch here?

I have no idea.  I modeled this after the postdata code, basically.  The obvious thing would be the Write() method of the object failing for some reason.

> I'd prefer if you used a different property (aEntry.postdata_b64 or similar)

OK, I can do that.

> Aren't JavaScript strings able to contain \0 ?

They are, but the JSON serialization doesn't deal.  See bug 396068.  There might be similar issues with other control chars, so this seemed like the safe thing to do.

> Again: what will happen here when aEntry.owner is a nsIURI.spec from Firefox
> 2?

I suppose we can use ownerURI on branch and create a principal here... Would that work? 

Too bad our security APIs are such a mess.  :(

Comment 11

10 years ago
(In reply to comment #10)
> I have no idea.

Catching seems like the safer thing to do, but please add a comment as to that this isn't anything special you're trying to catch (in case the code gets refactored or we want to try without the try-catch-block).

> They are, but the JSON serialization doesn't deal.  See bug 396068.

Hm, our JSON code would really have dealt with that more graciously than toSource which we use here. Fixing bug 387859 would thus prevent the need for this hack. Please add a comment as to considering reverting the postdata bits once that bug is fixed.

> I suppose we can use ownerURI on branch and create a principal here...

That'd be great. Any chance your trunk patch could make use of .ownerURI as well when .owner is missing?

BTW: How portable are the serialized owners (Mac/Windows/different profiles)?
> Please add a comment as to considering reverting the postdata bits

Some of those bits are needed no matter what (e.g. the setData() length changes).  But yes, the base64 encoding would not be needed.

> Any chance your trunk patch could make use of .ownerURI as
> well when .owner is missing?

That's what I meant by "create a principal here".  ;)

> BTW: How portable are the serialized owners (Mac/Windows/different profiles)?

They're as portable as anything else we send through the binary input stream.  Which means that things will work unless someone changes the serialization functions in nsPrincipal, will be portable across different endianness machines, etc.

Updated

10 years ago
Attachment #280766 - Flags: superreview?(jst)
Attachment #280766 - Flags: superreview+
Attachment #280766 - Flags: review?(jst)
Attachment #280766 - Flags: review+

Updated

10 years ago
Attachment #280767 - Flags: superreview?(jst)
Attachment #280767 - Flags: superreview+
Attachment #280767 - Flags: review?(jst)
Attachment #280767 - Flags: review+
Depends on: 396389
Created attachment 281132 [details] [diff] [review]
Updated branch patch

Let me know if I need a different reviewer, ok?
Attachment #281132 - Flags: review?(zeniko)
Created attachment 281133 [details] [diff] [review]
Updated trunk fix

I've tested that Fx2 -> Fx2, Fx2 -> trunk, trunk -> trunk restores all work with these patches.
Attachment #280766 - Attachment is obsolete: true
Attachment #280767 - Attachment is obsolete: true
Attachment #281133 - Flags: review?(zeniko)
Attachment #280766 - Flags: review?(mconnor)
Attachment #280767 - Flags: review?(mconnor)

Updated

10 years ago
Attachment #281132 - Flags: review?(zeniko) → review+

Comment 15

10 years ago
Comment on attachment 281133 [details] [diff] [review]
Updated trunk fix

r+ for the SessionStore changes. Just one detail:

>+          // We can stop doing base64 encoding once our serialization into JSON
>+          // is guaranteed to handle all chars in strings, including embedded
>+          // nulls.

JSON and toSource/uneval are two quite similar but still different serializations - and it's only the latter which fails here. What about "... once .toSource() is guaranteed ... (see bug 375639)."?
Attachment #281133 - Flags: review?(zeniko) → review+
I can do that, sure.  Though I wouldn't trust it to deal once that bug is fixed....
Comment on attachment 281132 [details] [diff] [review]
Updated branch patch

Requesting branch approval.  This should be a very safe fix that fixes corner cases of session restore and about:blank subframes in most cases (basically all cases except when the parent page has a certificate principal).
Attachment #281132 - Flags: approval1.8.1.8?
Comment on attachment 281133 [details] [diff] [review]
Updated trunk fix

Requesting approval for regression fix.  Risk is pretty low, in my opinion.
Attachment #281133 - Flags: approval1.9?

Updated

10 years ago
Attachment #281133 - Flags: approval1.9? → approval1.9+
Trunk fix checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 281132 [details] [diff] [review]
Updated branch patch

approved for 1.8.1.8, a=dveditz for release-drivers
Attachment #281132 - Flags: approval1.8.1.8? → approval1.8.1.8+
Fixed on branch
Keywords: fixed1.8.1.8
verified fixed 1.8.1.8 using Mozilla/5.0 (Windows; U; Windows NT 5.2; de; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8 ID:2007100816 and the steps to reproduce from this bug 
Keywords: fixed1.8.1.8 → verified1.8.1.8

Updated

10 years ago
Duplicate of this bug: 385882

Updated

10 years ago
Duplicate of this bug: 344185
You need to log in before you can comment on or make changes to this bug.